[FFmpeg-devel] [PATCH 2/2] pngdec: decode and expose iCCP chunks as side data

[Rostislav Pehlivanov]

On 23 July 2017 at 13:36, Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Sat, Jul 22, 2017 at 09:15:53PM +0100, Rostislav Pehlivanov wrote:
> > On 21 July 2017 at 14:11, Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> >
> > > On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote:
> > > > Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
> > > > ---
> > > >  libavcodec/pngdec.c | 45 ++++++++++++++++++++++++++++++
> +++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > >
> > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > index 083f61f4f8..64811c6fc3 100644
> > > > --- a/libavcodec/pngdec.c
> > > > +++ b/libavcodec/pngdec.c
> > > > @@ -836,6 +836,46 @@ static int decode_trns_chunk(AVCodecContext
> > > *avctx, PNGDecContext *s,
> > > >      return 0;
> > > >  }
> > > >
> > > > +static int decode_iccp_chunk(PNGDecContext *s, uint32_t length,
> > > AVFrame *f)
> > > > +{
> > > > +    int ret, cnt = 0;
> > > > +    uint8_t *data, profile_name[82];
> > > > +    AVBPrint bp;
> > > > +    AVFrameSideData *sd;
> > > > +
> > > > +    while ((profile_name[cnt++] = bytestream2_get_byte(&s->gb)) &&
> cnt
> > > < 81);
> > > > +    if (cnt > 80) {
> > > > +        av_log(s->avctx, AV_LOG_ERROR, "iCCP with invalid name!\n");
> > > > +        return AVERROR_INVALIDDATA;
> > > > +    }
> > > > +
> > > > +    length -= cnt;
> > > > +
> > > > +    if (bytestream2_get_byte(&s->gb) != 0) {
> > > > +        av_log(s->avctx, AV_LOG_ERROR, "iCCP with invalid
> > > compression!\n");
> > > > +        return AVERROR_INVALIDDATA;
> > > > +    }
> > > > +
> > > > +    length -= 1;
> > >
> > > length could have overflowed and become rather big from one of the 2
> > > subtractions
> > > the following code would then misbehave
> > >
> > >
> > Thanks to pointing this out
> >
> > Changed to:
> > + length = FFMAX(length - cnt, 0);
>
> if length is unsigned int and cnt signed int then length - cnt is unsigned
> so the FFMAX will not work as intended
>
>
> > and
> > + length = FFMAX(length - 1, 0);
> >
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Changed to an int as discussed on IRC

I'll give it a few more hours before I push both patches