[FFmpeg-devel] [PATCH] avcodec/v4l2: fix access to priv_data after codec close.

[Mark Thompson]

On 17/10/17 18:20, Jorge Ramirez-Ortiz wrote:
> A user can close the codec while keeping references to some of the
> capture buffers. When the codec is closed, the structure that keeps
> the contexts, state and the driver file descriptor is freed.
> 
> Since access to some of the elements in that structure is required to
> properly release the memory during the buffer unref callbacks, the
> following commit makes sure the unref callback accesses valid memory.
> 
> This commit was tested with valgrind:
> 
> $ valgrind ffmpeg_g -report -threads 1 -v 55  -y -c:v h264_v4l2m2m \
>   -i video.h264 -an -frames:v 100 -f null -
> ---
>  libavcodec/v4l2_buffers.c | 17 ++++++++++++++++-
>  libavcodec/v4l2_buffers.h |  6 ++++++
>  libavcodec/v4l2_m2m.c     | 29 ++++++++++++++++++++++++-----
>  libavcodec/v4l2_m2m_dec.c |  2 +-
>  4 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index ba70c5d..80e65ca 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -205,9 +205,24 @@ static enum AVColorTransferCharacteristic v4l2_get_color_trc(V4L2Buffer *buf)
>  static void v4l2_free_buffer(void *opaque, uint8_t *unused)
>  {
>      V4L2Buffer* avbuf = opaque;
> -    V4L2m2mContext *s = buf_to_m2mctx(avbuf);
> +    V4L2m2mContext *s = avbuf->m2m ? avbuf->m2m : buf_to_m2mctx(avbuf);
>  
>      atomic_fetch_sub_explicit(&s->refcount, 1, memory_order_acq_rel);
> +
> +    if (avbuf->m2m) {
> +        if (!atomic_load(&s->refcount)) {
> +            /* unmmap and free the associated buffers */
> +            ff_v4l2_context_release(&s->capture);
> +
> +            /* close the v4l2 driver */
> +            close(s->fd);
> +
> +            /* release the duplicated m2m context */
> +            av_freep(&s);
> +        }
> +        return;
> +    }
> +
>      if (s->reinit) {
>          if (!atomic_load(&s->refcount))
>              sem_post(&s->refsync);
> diff --git a/libavcodec/v4l2_buffers.h b/libavcodec/v4l2_buffers.h
> index e28a4a6..d774480 100644
> --- a/libavcodec/v4l2_buffers.h
> +++ b/libavcodec/v4l2_buffers.h
> @@ -41,6 +41,12 @@ typedef struct V4L2Buffer {
>      /* each buffer needs to have a reference to its context */
>      struct V4L2Context *context;
>  
> +    /* when the codec is closed while the user has buffer references we
> +     * still need access to context data (this is a pointer to a duplicated
> +     * m2m context created during the codec close function).
> +     */
> +    struct V4L2m2mContext *m2m;
> +
>      /* keep track of the mmap address and mmap length */
>      struct V4L2Plane_info {
>          int bytesperline;
> diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
> index 1d7a852..91b1bbb 100644
> --- a/libavcodec/v4l2_m2m.c
> +++ b/libavcodec/v4l2_m2m.c
> @@ -313,8 +313,8 @@ error:
>  
>  int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
>  {
> -    V4L2m2mContext* s = avctx->priv_data;
> -    int ret;
> +    V4L2m2mContext *m2m, *s = avctx->priv_data;
> +    int i, ret;
>  
>      ret = ff_v4l2_context_set_status(&s->output, VIDIOC_STREAMOFF);
>      if (ret)
> @@ -325,12 +325,31 @@ int ff_v4l2_m2m_codec_end(AVCodecContext *avctx)
>          av_log(avctx, AV_LOG_ERROR, "VIDIOC_STREAMOFF %s\n", s->capture.name);
>  
>      ff_v4l2_context_release(&s->output);
> +    sem_destroy(&s->refsync);
>  
> -    if (atomic_load(&s->refcount))
> -        av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end leaving pending buffers\n");
> +    if (atomic_load(&s->refcount)) {
> +        av_log(avctx, AV_LOG_DEBUG, "ff_v4l2m2m_codec_end, referenced buffers %d\n", atomic_load(&s->refcount));
> +
> +        /* We are about to free the private data while the user still has references to the buffers.
> +         * This means that when the user releases those buffers, the v4l2_free_buffer callback will be pointing to free'd memory.
> +         * Duplicate the m2m context and update the buffers.
> +         */
> +        m2m = av_mallocz(sizeof(*m2m));
> +        if (m2m) {
> +            memcpy(m2m, s, sizeof(V4L2m2mContext));
> +            for (i = 0; i < s->capture.num_buffers; i++)
> +                s->capture.buffers[i].m2m = m2m;
> +
> +            return 0;
> +        }

No.

The user can call av_buffer_unref() and therefore v4l2_free_buffer() asynchronously on another thread while this manoeuvre is in progress.

> +
> +        /* on ENOMEM let's at least close the v4l2 driver and release the capture memory
> +         * so the driver is left in a healthy state.
> +         */
> +        av_log(avctx, AV_LOG_ERROR, "ff_v4l2m2m_codec_end, referenced buffers not accessible\n");
> +    }
>  
>      ff_v4l2_context_release(&s->capture);
> -    sem_destroy(&s->refsync);
>  
>      /* release the hardware */
>      if (close(s->fd) < 0 )
> diff --git a/libavcodec/v4l2_m2m_dec.c b/libavcodec/v4l2_m2m_dec.c
> index 958cdc5..f88f819 100644
> --- a/libavcodec/v4l2_m2m_dec.c
> +++ b/libavcodec/v4l2_m2m_dec.c
> @@ -38,7 +38,7 @@ static int v4l2_try_start(AVCodecContext *avctx)
>      V4L2m2mContext *s = avctx->priv_data;
>      V4L2Context *const capture = &s->capture;
>      V4L2Context *const output = &s->output;
> -    struct v4l2_selection selection;
> +    struct v4l2_selection selection = { 0 };

This looks like it should be a separate change.

>      int ret;
>  
>      /* 1. start the output process */
>