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

[Michael Niedermayer]

On Wed, Oct 18, 2017 at 09:46:40AM +0200, Jorge Ramirez wrote:
> On 10/18/2017 12:34 AM, Mark Thompson wrote:
> >>  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.
> >
> 
> right. as a matter of fact, this synchronization can not be done
> using private data (the opaque structure passed to the callback must
> contain all the required information (and updated!) at any time). On
> top of this being hard to implement would complicate the current
> code quite a bit making it also very hard to maintain. So agreed.
> 
> So back to a previous proposal, would the flag below be acceptable? [1]
> 
> Seems sensible to me to give responsibility of the private data
> deallocation to the codec itself independently of the close function
> since the buffers allocated by the codec controlled by a separate
> thread will need access to that data.
> 
> Put differently since the close function can not be made responsible
> for deallocating mmap/queued memory in V4L2 (or even closing the
> kernel driver) when the user keeps buffers, it should not be allowed
> to delete the codec private data since in my view it forces the
> design to take a lot of unnecessary actions for no real reason.
> 
> With the proposal below, the code remains cleaner and free of race
> conditions.
> 
> [1] ..
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 52cc5b0..0a3ca0c 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -982,6 +982,11 @@ typedef struct RcOverride{
>   * Do not reset ASS ReadOrder field on flush (subtitles decoding)
>   */
>  #define AV_CODEC_FLAG2_RO_FLUSH_NOOP  (1 << 30)
> +/**
> + * Closing the codec doesnt release the context priv_data (it
> becomes the codec
> + * responsibility)
> + */
> +#define AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE  (1 << 31)
> 
>  /* Unsupported options :
>   *              Syntax Arithmetic coding (SAC)
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 9551f31..b6c5adf 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1211,7 +1211,9 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>      if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
>          av_opt_free(avctx->priv_data);
>      av_opt_free(avctx);
> -    av_freep(&avctx->priv_data);
> +    if (!(avctx->flags2 & AV_CODEC_FLAG2_PRESERVE_PRIVDATA_ONCLOSE))
> +        av_freep(&avctx->priv_data);


This may be solving a similar issue as our udp.c cleanup with its
thread. Not sure, i didnt follow this discussion closely.
just wanted to mention as one migt benefit from the other if its the
same issue.

Another thing that came to my mind, if the context (and other resources)
are deallocated with a delay beyond closing. Is this still correct if
the process terminates before or in the middle.
Or is that preented somehow ?

There would be at least spurious warnings from memory debuggers if
deallocation isnt run at all

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171018/20fa3763/attachment.sig>