[FFmpeg-devel] [PATCH 5/6] pngdec: fix and simplify apng reference handling

[Michael Niedermayer]

On Tue, Mar 30, 2021 at 10:56:13AM +0200, Michael Niedermayer wrote:
> On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote:
> > Current code is very confused and confusing. It uses two different
> > reference frames - "previous" and "last" - when only one is really
> > necessary. It also confuses the two, leading to incorrect output with
> > APNG_DISPOSE_OP_PREVIOUS mode.
> > 
> > Fixes #9017.
> > ---
> >  libavcodec/pngdec.c | 93 ++++++++++++++++++++-------------------------
> >  1 file changed, 42 insertions(+), 51 deletions(-)
> 
> 
> [...]
> 
> > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
> >      if (!buffer)
> >          return AVERROR(ENOMEM);
> >  
> > +    ff_thread_await_progress(&s->last_picture, INT_MAX, 0);
> >  
> > -    // Do the disposal operation specified by the last frame on the frame
> > -    if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) {
> > -        ff_thread_await_progress(&s->last_picture, INT_MAX, 0);
> > -        memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height);
> > -
> > -        if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND)
> > -            for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y)
> > -                memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w);
> > +    // need to reset a rectangle to background:
> > +    // create a new writable copy
> > +    if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) {
> > +        int ret = av_frame_make_writable(s->last_picture.f);
> > +        if (ret < 0)
> > +            return ret;
> >  
> > -        memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height);
> > -        ff_thread_report_progress(&s->previous_picture, INT_MAX, 0);
> > -    } else {
> > -        ff_thread_await_progress(&s->previous_picture, INT_MAX, 0);
> > -        memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height);
> > +        for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) {
> > +            memset(s->last_picture.f->data[0] + s->image_linesize * y +
> > +                   s->bpp * s->last_x_offset, 0, s->bpp * s->last_w);
> > +        }
> >      }
> >  
> > +    memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height);
> > +
> 
> This results in out of array reads
> 
> av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup()
> assumes all frames have the same linesize

CCing anton, to make sure this is not missed in its old thread
A fix to this also should be backported to release/4.4

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210330/e4c01349/attachment.sig>