A potential Use-After-Free bug in the source file libavfilter/vf_hwmap.c

[wurongxin]

Summary of the bug:
How to reproduce:

% ffmpeg -i input ... output
ffmpeg version
built on ...

Patches should be submitted to the ffmpeg-devel mailing list and not this bug tracker.

In the source file ​​https://github.com/FFmpeg/FFmpeg/blob/master/libavfilter/vf_hwmap.c, at Line 114, the call to "av_hwframe_ctx_create_derived" would free device->buffer, and device->buffer would be used later at Line 259. This lead to a use-after-free bug.

53.	static int hwmap_config_output(AVFilterLink *outlink)
54.	{
	      …
114.	            err = av_hwframe_ctx_create_derived(&ctx->hwframes_ref,
115.	                                                outlink->format,
116.	                                                device,
117.	                                                inlink->hw_frames_ctx,
118.	                                                ctx->mode);
119.	            if (err < 0) {
120.	                av_log(avctx, AV_LOG_ERROR, "Failed to create derived "
121.	                       "frames context: %d.\n", err);
122.	                goto fail;
123.	            }
	     …
257.	fail:
258.	    if (device_is_derived)
259.	        av_buffer_unref(&device);
260.	    av_buffer_unref(&ctx->hwframes_ref);
261.	    return err;

To see how "av_hwframe_ctx_create_derived" would free device->buffer, please see the following code snippet in the source file libavutil/hwcontext.c. At Line 828, the copy of the variable derived_device_ctx will be created and assigned to the variable dst_ref. Since this copy is a shallow copy, dst_ref->buffer is actually the same memory address as derived_device_ctx->buffer. At Line 870, dst_ref->buffer can be freed when calling to the function av_buffer_unref.

798.	int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx,
799.	                                  enum AVPixelFormat format,
800.	                                  AVBufferRef *derived_device_ctx,
801.	                                  AVBufferRef *source_frame_ctx,
802.	                                  int flags)
803.	{
	…
828.	    dst_ref = av_hwframe_ctx_alloc(derived_device_ctx);
	…
867.	fail:
868. 	    if (dst)
869.	        av_buffer_unref(&dst->internal->source_frames);
870.	    av_buffer_unref(&dst_ref);
871.	    return ret;

The code snippet about the function av_buffer_unref is shown as follows.

void av_buffer_unref(AVBufferRef **buf)
{
    if (!buf || !*buf)
        return;

    buffer_replace(buf, NULL);
}

static void buffer_replace(AVBufferRef **dst, AVBufferRef **src)
{
    AVBuffer *b;

    b = (*dst)->buffer;

    if (src) {
        **dst = **src;
        av_freep(src);
    } else
        av_freep(dst);

    if (atomic_fetch_add_explicit(&b->refcount, -1, memory_order_acq_rel) == 1) {
        b->free(b->opaque, b->data);
        av_freep(&b);
    }
}

[mkver]

At Line 828, the copy of the variable derived_device_ctx will be created and assigned to the variable dst_ref. Since this copy is a shallow copy, dst_ref->buffer is actually the same memory address as derived_device_ctx->buffer. At Line 870, dst_ref->buffer can be freed when calling to the function av_buffer_unref.

No.

1. dst_ref->buffer is not the same as derived_device_ctx->buffer; ((AVHWFramesContext*) dst_ref->buffer)->device_ref->buffer is.
2. But more importantly, you completely ignored/misunderstood that we are dealing with reference counted buffers here:
   a) If the call to av_hwframe_ctx_alloc() fails, the reference counter of the underlying AVBuffer of derived_device_ctx is the same as before the call and dst_ref is NULL, so that av_buffer_unref(&dst_ref) is basically a no-op.
   b) If the call to av_hwframe_ctx_alloc() succeeds, the reference counter to derived_device_ctx has been incremented by 1. Should we goto fail in av_hwframe_ctx_create_derived() lateron, dst_ref will be unreferenced; given that the reference counter of the underlying AVBuffer (which is different from the underlying AVBuffer of derived_device_ctx) is certain to be 1 at this point, this will trigger freeing of the underlying buffer via hwframe_ctx_free(). This in turn will unreference ((AVHWFramesContext*) dst_ref->buffer)->device_ref and therefore decrement the reference counter of the underlying buffer of derived_device_ctx, but it will not free this buffer; it will just undo the earlier increment.