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

[Rostislav Pehlivanov]

On 25 July 2017 at 17:33, Rostislav Pehlivanov <atomnuker at gmail.com> wrote:

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

Thanks for the review, pushed with your suggestions.