diff mbox series

[FFmpeg-devel] avcodec/pngdec: fix possible race condition with APNG decoding

Message ID 20210211215833.3133-1-onemda@gmail.com
State Accepted
Commit 63231fa8d30f41045658d6c382b00fe1ebe18d05
Headers show
Series [FFmpeg-devel] avcodec/pngdec: fix possible race condition with APNG decoding | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Paul B Mahol Feb. 11, 2021, 9:58 p.m. UTC 
Fixes #9017

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/pngdec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Anton Khirnov Feb. 14, 2021, 10:20 a.m. UTC | #1 
Quoting Paul B Mahol (2021-02-11 22:58:33)
> Fixes #9017
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/pngdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 395b86bbe7..61642b7cbe 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
>              s->bpp += byte_depth;
>          }
>  
> -        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> -            return ret;
>          if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) {
>              ff_thread_release_buffer(avctx, &s->previous_picture);
>              if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
>                  return ret;
>          }
> +        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> +            return ret;
>          p->pict_type        = AV_PICTURE_TYPE_I;
>          p->key_frame        = 1;
>          p->interlaced_frame = !!s->interlace_type;
> -- 
> 2.17.1

It's pretty non-obvious what the race is and how is it fixed by
reordering the calls.
Paul B Mahol Feb. 14, 2021, 10:41 a.m. UTC | #2 
On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Paul B Mahol (2021-02-11 22:58:33)
> > Fixes #9017
> >
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavcodec/pngdec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > index 395b86bbe7..61642b7cbe 100644
> > --- a/libavcodec/pngdec.c
> > +++ b/libavcodec/pngdec.c
> > @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext
> *avctx, PNGDecContext *s,
> >              s->bpp += byte_depth;
> >          }
> >
> > -        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> AV_GET_BUFFER_FLAG_REF)) < 0)
> > -            return ret;
> >          if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op
> != APNG_DISPOSE_OP_PREVIOUS) {
> >              ff_thread_release_buffer(avctx, &s->previous_picture);
> >              if ((ret = ff_thread_get_buffer(avctx,
> &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> >                  return ret;
> >          }
> > +        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> AV_GET_BUFFER_FLAG_REF)) < 0)
> > +            return ret;
> >          p->pict_type        = AV_PICTURE_TYPE_I;
> >          p->key_frame        = 1;
> >          p->interlaced_frame = !!s->interlace_type;
> > --
> > 2.17.1
>
> It's pretty non-obvious what the race is and how is it fixed by
> reordering the calls.
>

Before patch hash of decoded output would differ with single threading and
multiple threads.
Now it does not.

>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 14, 2021, 10:43 a.m. UTC | #3 
Quoting Paul B Mahol (2021-02-14 11:41:20)
> On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Paul B Mahol (2021-02-11 22:58:33)
> > > Fixes #9017
> > >
> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > ---
> > >  libavcodec/pngdec.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > index 395b86bbe7..61642b7cbe 100644
> > > --- a/libavcodec/pngdec.c
> > > +++ b/libavcodec/pngdec.c
> > > @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext
> > *avctx, PNGDecContext *s,
> > >              s->bpp += byte_depth;
> > >          }
> > >
> > > -        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > -            return ret;
> > >          if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op
> > != APNG_DISPOSE_OP_PREVIOUS) {
> > >              ff_thread_release_buffer(avctx, &s->previous_picture);
> > >              if ((ret = ff_thread_get_buffer(avctx,
> > &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> > >                  return ret;
> > >          }
> > > +        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > +            return ret;
> > >          p->pict_type        = AV_PICTURE_TYPE_I;
> > >          p->key_frame        = 1;
> > >          p->interlaced_frame = !!s->interlace_type;
> > > --
> > > 2.17.1
> >
> > It's pretty non-obvious what the race is and how is it fixed by
> > reordering the calls.
> >
> 
> Before patch hash of decoded output would differ with single threading and
> multiple threads.
> Now it does not.

That still does not explain where the bug is and why this specific
change is the right way to fix it.
Paul B Mahol Feb. 14, 2021, 10:48 a.m. UTC | #4 
On Sun, Feb 14, 2021 at 11:44 AM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Paul B Mahol (2021-02-14 11:41:20)
> > On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net>
> wrote:
> >
> > > Quoting Paul B Mahol (2021-02-11 22:58:33)
> > > > Fixes #9017
> > > >
> > > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > > ---
> > > >  libavcodec/pngdec.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > index 395b86bbe7..61642b7cbe 100644
> > > > --- a/libavcodec/pngdec.c
> > > > +++ b/libavcodec/pngdec.c
> > > > @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext
> > > *avctx, PNGDecContext *s,
> > > >              s->bpp += byte_depth;
> > > >          }
> > > >
> > > > -        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > > -            return ret;
> > > >          if (avctx->codec_id == AV_CODEC_ID_APNG &&
> s->last_dispose_op
> > > != APNG_DISPOSE_OP_PREVIOUS) {
> > > >              ff_thread_release_buffer(avctx, &s->previous_picture);
> > > >              if ((ret = ff_thread_get_buffer(avctx,
> > > &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> > > >                  return ret;
> > > >          }
> > > > +        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > > +            return ret;
> > > >          p->pict_type        = AV_PICTURE_TYPE_I;
> > > >          p->key_frame        = 1;
> > > >          p->interlaced_frame = !!s->interlace_type;
> > > > --
> > > > 2.17.1
> > >
> > > It's pretty non-obvious what the race is and how is it fixed by
> > > reordering the calls.
> > >
> >
> > Before patch hash of decoded output would differ with single threading
> and
> > multiple threads.
> > Now it does not.
>
> That still does not explain where the bug is and why this specific
> change is the right way to fix it.
>

Speedup gain is still there with multiple threads.
And when using helgrind one of items was gone and no new items were
introduced.


>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov Feb. 14, 2021, 10:51 a.m. UTC | #5 
Quoting Paul B Mahol (2021-02-14 11:48:14)
> On Sun, Feb 14, 2021 at 11:44 AM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Paul B Mahol (2021-02-14 11:41:20)
> > > On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net>
> > wrote:
> > >
> > > > Quoting Paul B Mahol (2021-02-11 22:58:33)
> > > > > Fixes #9017
> > > > >
> > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > > > ---
> > > > >  libavcodec/pngdec.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > > index 395b86bbe7..61642b7cbe 100644
> > > > > --- a/libavcodec/pngdec.c
> > > > > +++ b/libavcodec/pngdec.c
> > > > > @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext
> > > > *avctx, PNGDecContext *s,
> > > > >              s->bpp += byte_depth;
> > > > >          }
> > > > >
> > > > > -        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > > > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > > > -            return ret;
> > > > >          if (avctx->codec_id == AV_CODEC_ID_APNG &&
> > s->last_dispose_op
> > > > != APNG_DISPOSE_OP_PREVIOUS) {
> > > > >              ff_thread_release_buffer(avctx, &s->previous_picture);
> > > > >              if ((ret = ff_thread_get_buffer(avctx,
> > > > &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
> > > > >                  return ret;
> > > > >          }
> > > > > +        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> > > > AV_GET_BUFFER_FLAG_REF)) < 0)
> > > > > +            return ret;
> > > > >          p->pict_type        = AV_PICTURE_TYPE_I;
> > > > >          p->key_frame        = 1;
> > > > >          p->interlaced_frame = !!s->interlace_type;
> > > > > --
> > > > > 2.17.1
> > > >
> > > > It's pretty non-obvious what the race is and how is it fixed by
> > > > reordering the calls.
> > > >
> > >
> > > Before patch hash of decoded output would differ with single threading
> > and
> > > multiple threads.
> > > Now it does not.
> >
> > That still does not explain where the bug is and why this specific
> > change is the right way to fix it.
> >
> 
> Speedup gain is still there with multiple threads.
> And when using helgrind one of items was gone and no new items were
> introduced.

That still does not explain where the bug was. If you used helgrind, you
might as well explain what it found.
diff mbox series

Patch

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 395b86bbe7..61642b7cbe 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -711,13 +711,13 @@  static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
             s->bpp += byte_depth;
         }
 
-        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
-            return ret;
         if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) {
             ff_thread_release_buffer(avctx, &s->previous_picture);
             if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
                 return ret;
         }
+        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
+            return ret;
         p->pict_type        = AV_PICTURE_TYPE_I;
         p->key_frame        = 1;
         p->interlaced_frame = !!s->interlace_type;