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

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(-)

On Tue, 17 Oct 2017 19:20:50 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> 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;
> +        }
> +
> +        /* 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");
> +    }
>  

This is getting pretty messy. The better way to handle this would be
moving all state into a separate struct, and refcounting it with
AVBufferRefs. No more atomics, no more semaphores, no more blowing up
on OOM.

(Actually the hwcontext stuff (AVHWFramesContext etc.) provides an
infrastructure for this, and all hwaccels use it to avoid such issues.)

Anyway, not blocking this patch...

>      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 };
>      int ret;
>  
>      /* 1. start the output process */

Stray change?

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 */
>