| Message ID | 20170720204622.27337-2-atomnuker@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote: > Signed-off-by: Rostislav Pehlivanov <atomnuker@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 > + > + if ((ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length) < 0)) () is placed incorrectly > + return ret; > + > + av_bprint_finalize(&bp, (char **)&data); > + > + if (!(sd = av_frame_new_side_data(f, AV_FRAME_DATA_ICC_PROFILE, bp.len))) > + return AVERROR(ENOMEM); > + > + av_dict_set(&sd->metadata, "name", profile_name, 0); > + memcpy(sd->data, data, bp.len); > + av_free(data); > + > + /* ICC compressed data and CRC */ > + bytestream2_skip(&s->gb, length + 4); > + > + return 0; > +} > + > static void handle_small_bpp(PNGDecContext *s, AVFrame *p) > { > if (s->bits_per_pixel == 1 && s->color_type == PNG_COLOR_TYPE_PALETTE) { > @@ -1239,6 +1279,11 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, > bytestream2_skip(&s->gb, 4); /* crc */ > break; > } > + case MKTAG('i', 'C', 'C', 'P'): { > + if (decode_iccp_chunk(s, length, p) < 0) > + goto fail; > + break; > + } > case MKTAG('I', 'E', 'N', 'D'): > if (!(s->pic_state & PNG_ALLIMAGE)) > av_log(avctx, AV_LOG_ERROR, "IEND without all image\n"); > -- > 2.14.0.rc0.284.gd933b75aa4 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 21 July 2017 at 14:11, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote: > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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); and + length = FFMAX(length - 1, 0); > > > + > > + if ((ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length) < > 0)) > > () is placed incorrectly > > Changed to: + ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length); + if (ret < 0)
On Sat, Jul 22, 2017 at 09:15:53PM +0100, Rostislav Pehlivanov wrote: > On 21 July 2017 at 14:11, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote: > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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); > [...]
On 23 July 2017 at 13:36, Michael Niedermayer <michael@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@niedermayer.cc> > > wrote: > > > > > On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote: > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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
On 25 July 2017 at 17:33, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > > > On 23 July 2017 at 13:36, Michael Niedermayer <michael@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@niedermayer.cc> >> > wrote: >> > >> > > On Thu, Jul 20, 2017 at 09:46:22PM +0100, Rostislav Pehlivanov wrote: >> > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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.
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; + + if ((ret = decode_zbuf(&bp, s->gb.buffer, s->gb.buffer + length) < 0)) + return ret; + + av_bprint_finalize(&bp, (char **)&data); + + if (!(sd = av_frame_new_side_data(f, AV_FRAME_DATA_ICC_PROFILE, bp.len))) + return AVERROR(ENOMEM); + + av_dict_set(&sd->metadata, "name", profile_name, 0); + memcpy(sd->data, data, bp.len); + av_free(data); + + /* ICC compressed data and CRC */ + bytestream2_skip(&s->gb, length + 4); + + return 0; +} + static void handle_small_bpp(PNGDecContext *s, AVFrame *p) { if (s->bits_per_pixel == 1 && s->color_type == PNG_COLOR_TYPE_PALETTE) { @@ -1239,6 +1279,11 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, bytestream2_skip(&s->gb, 4); /* crc */ break; } + case MKTAG('i', 'C', 'C', 'P'): { + if (decode_iccp_chunk(s, length, p) < 0) + goto fail; + break; + } case MKTAG('I', 'E', 'N', 'D'): if (!(s->pic_state & PNG_ALLIMAGE)) av_log(avctx, AV_LOG_ERROR, "IEND without all image\n");
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> --- libavcodec/pngdec.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)