Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Sat, May 13, 2017 at 08:38:52AM +0200, wm4 wrote: > On Fri, 12 May 2017 20:55:13 +0200 > Michael Niedermayerwrote: > > > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > > > On 5/12/17, Michael Niedermayer wrote: > > > > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > > > >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > > > >> > On 5/11/17, Michael Niedermayer wrote: > > > >> > > Most of these were found through code review in response to > > > >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > > > >> > > There is thus no testcase for most of this. > > > >> > > The initial issue was Found-by: continuous fuzzing process > > > >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > >> > > > > > >> > > Signed-off-by: Michael Niedermayer > > > >> > > --- > > > >> > > libavcodec/xpmdec.c | 37 ++--- > > > >> > > 1 file changed, 30 insertions(+), 7 deletions(-) > > > >> > > > > > >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > > > >> > > index 9112d4cb5e..03172e4aad 100644 > > > >> > > --- a/libavcodec/xpmdec.c > > > >> > > +++ b/libavcodec/xpmdec.c > > > >> > > @@ -29,6 +29,8 @@ > > > >> > > typedef struct XPMContext { > > > >> > > uint32_t *pixels; > > > >> > > intpixels_size; > > > >> > > +uint8_t *buf; > > > >> > > +intbuf_size; > > > >> > > } XPMDecContext; > > > >> > > > > > >> > > typedef struct ColorEntry { > > > >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char > > > >> > > *p, int > > > >> > > len) > > > >> > > const ColorEntry *entry; > > > >> > > char color_name[100]; > > > >> > > > > > >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > > > >> > > + > > > >> > > if (*p == '#') { > > > >> > > p++; > > > >> > > len--; > > > >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext > > > >> > > *avctx, > > > >> > > void *data, > > > >> > > { > > > >> > > XPMDecContext *x = avctx->priv_data; > > > >> > > AVFrame *p=data; > > > >> > > -const uint8_t *end, *ptr = avpkt->data; > > > >> > > +const uint8_t *end, *ptr; > > > >> > > int ncolors, cpp, ret, i, j; > > > >> > > int64_t size; > > > >> > > uint32_t *dst; > > > >> > > > > > >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > > > >> > > > > > >> > > -end = avpkt->data + avpkt->size; > > > >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > > > >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > > > >> > > +if (!x->buf) > > > >> > > +return AVERROR(ENOMEM); > > > >> > > +memcpy(x->buf, avpkt->data, avpkt->size); > > > >> > > +x->buf[avpkt->size] = 0; > > > >> > > + > > > >> > > +ptr = x->buf; > > > >> > > +end = x->buf + avpkt->size; > > > >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > > > >> > > ptr++; > > > >> > > > > > >> > > -if (ptr >= end) { > > > >> > > +if (end - ptr <= 9) { > > > >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > > > >> > > return AVERROR_INVALIDDATA; > > > >> > > } > > > >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext > > > >> > > *avctx, > > > >> > > void > > > >> > > *data, > > > >> > > > > > >> > > size = 1; > > > >> > > for (i = 0; i < cpp; i++) > > > >> > > -size *= 94; > > > >> > > +size *= 95; > > > >> > > > > > >> > > if (ncolors <= 0 || ncolors > size) { > > > >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: > > > >> > > %d\n", > > > >> > > ncolors); > > > >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext > > > >> > > *avctx, > > > >> > > void *data, > > > >> > > return AVERROR(ENOMEM); > > > >> > > > > > >> > > ptr += mod_strcspn(ptr, ",") + 1; > > > >> > > +if (end - ptr < 1) > > > >> > > +return AVERROR_INVALIDDATA; > > > >> > > + > > > >> > > for (i = 0; i < ncolors; i++) { > > > >> > > const uint8_t *index; > > > >> > > int len; > > > >> > > > > > >> > > ptr += mod_strcspn(ptr, "\"") + 1; > > > >> > > -if (ptr + cpp > end) > > > >> > > +if (end - ptr < cpp) > > > >> > > return AVERROR_INVALIDDATA; > > > >> > > index = ptr; > > > >> > > ptr += cpp; > > > >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext > > > >> > > *avctx, > > > >> > > void *data, > > > >> > > > > > >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); > > > >> > > ptr += mod_strcspn(ptr, ",") + 1; > > > >> > > +if (end - ptr < 1) > > > >> > > +return AVERROR_INVALIDDATA; > > > >> > > } > > > >> > > > > > >> > > for (i = 0; i < avctx->height;
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Fri, 12 May 2017 20:55:13 +0200 Michael Niedermayerwrote: > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > > On 5/12/17, Michael Niedermayer wrote: > > > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > > >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > > >> > On 5/11/17, Michael Niedermayer wrote: > > >> > > Most of these were found through code review in response to > > >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > > >> > > There is thus no testcase for most of this. > > >> > > The initial issue was Found-by: continuous fuzzing process > > >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > >> > > > > >> > > Signed-off-by: Michael Niedermayer > > >> > > --- > > >> > > libavcodec/xpmdec.c | 37 ++--- > > >> > > 1 file changed, 30 insertions(+), 7 deletions(-) > > >> > > > > >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > > >> > > index 9112d4cb5e..03172e4aad 100644 > > >> > > --- a/libavcodec/xpmdec.c > > >> > > +++ b/libavcodec/xpmdec.c > > >> > > @@ -29,6 +29,8 @@ > > >> > > typedef struct XPMContext { > > >> > > uint32_t *pixels; > > >> > > intpixels_size; > > >> > > +uint8_t *buf; > > >> > > +intbuf_size; > > >> > > } XPMDecContext; > > >> > > > > >> > > typedef struct ColorEntry { > > >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char > > >> > > *p, int > > >> > > len) > > >> > > const ColorEntry *entry; > > >> > > char color_name[100]; > > >> > > > > >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > > >> > > + > > >> > > if (*p == '#') { > > >> > > p++; > > >> > > len--; > > >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext > > >> > > *avctx, > > >> > > void *data, > > >> > > { > > >> > > XPMDecContext *x = avctx->priv_data; > > >> > > AVFrame *p=data; > > >> > > -const uint8_t *end, *ptr = avpkt->data; > > >> > > +const uint8_t *end, *ptr; > > >> > > int ncolors, cpp, ret, i, j; > > >> > > int64_t size; > > >> > > uint32_t *dst; > > >> > > > > >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > > >> > > > > >> > > -end = avpkt->data + avpkt->size; > > >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > > >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > > >> > > +if (!x->buf) > > >> > > +return AVERROR(ENOMEM); > > >> > > +memcpy(x->buf, avpkt->data, avpkt->size); > > >> > > +x->buf[avpkt->size] = 0; > > >> > > + > > >> > > +ptr = x->buf; > > >> > > +end = x->buf + avpkt->size; > > >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > > >> > > ptr++; > > >> > > > > >> > > -if (ptr >= end) { > > >> > > +if (end - ptr <= 9) { > > >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > > >> > > return AVERROR_INVALIDDATA; > > >> > > } > > >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext > > >> > > *avctx, > > >> > > void > > >> > > *data, > > >> > > > > >> > > size = 1; > > >> > > for (i = 0; i < cpp; i++) > > >> > > -size *= 94; > > >> > > +size *= 95; > > >> > > > > >> > > if (ncolors <= 0 || ncolors > size) { > > >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: > > >> > > %d\n", > > >> > > ncolors); > > >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext > > >> > > *avctx, > > >> > > void *data, > > >> > > return AVERROR(ENOMEM); > > >> > > > > >> > > ptr += mod_strcspn(ptr, ",") + 1; > > >> > > +if (end - ptr < 1) > > >> > > +return AVERROR_INVALIDDATA; > > >> > > + > > >> > > for (i = 0; i < ncolors; i++) { > > >> > > const uint8_t *index; > > >> > > int len; > > >> > > > > >> > > ptr += mod_strcspn(ptr, "\"") + 1; > > >> > > -if (ptr + cpp > end) > > >> > > +if (end - ptr < cpp) > > >> > > return AVERROR_INVALIDDATA; > > >> > > index = ptr; > > >> > > ptr += cpp; > > >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext > > >> > > *avctx, > > >> > > void *data, > > >> > > > > >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); > > >> > > ptr += mod_strcspn(ptr, ",") + 1; > > >> > > +if (end - ptr < 1) > > >> > > +return AVERROR_INVALIDDATA; > > >> > > } > > >> > > > > >> > > for (i = 0; i < avctx->height; i++) { > > >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); > > >> > > +if (end - ptr < 1) > > >> > > +return AVERROR_INVALIDDATA; > > >> > > ptr += mod_strcspn(ptr, "\"") + 1; > > >> > > +if (end - ptr < 1) > > >> > > +
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Fri, 12 May 2017 18:21:07 +0200 Nicolas Georgewrote: > Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit : > > I told you that its unacceptable. > > And I told you, before you pushed, that your cowboy-style parser was > unacceptable. You chose to disregard it. AFAIK all of our subtitle parsers make the same assumption. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Fri, May 12, 2017 at 09:13:51PM +0200, Paul B Mahol wrote: > On 5/12/17, Paul B Maholwrote: > > On 5/12/17, Michael Niedermayer wrote: > >> On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > >>> On 5/12/17, Michael Niedermayer wrote: > >>> > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > >>> >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > >>> >> > On 5/11/17, Michael Niedermayer wrote: > >>> >> > > Most of these were found through code review in response to > >>> >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > >>> >> > > There is thus no testcase for most of this. > >>> >> > > The initial issue was Found-by: continuous fuzzing process > >>> >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > >>> >> > > > >>> >> > > Signed-off-by: Michael Niedermayer > >>> >> > > --- > >>> >> > > libavcodec/xpmdec.c | 37 ++--- > >>> >> > > 1 file changed, 30 insertions(+), 7 deletions(-) > >>> >> > > > >>> >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > >>> >> > > index 9112d4cb5e..03172e4aad 100644 > >>> >> > > --- a/libavcodec/xpmdec.c > >>> >> > > +++ b/libavcodec/xpmdec.c > >>> >> > > @@ -29,6 +29,8 @@ > >>> >> > > typedef struct XPMContext { > >>> >> > > uint32_t *pixels; > >>> >> > > intpixels_size; > >>> >> > > +uint8_t *buf; > >>> >> > > +intbuf_size; > >>> >> > > } XPMDecContext; > >>> >> > > > >>> >> > > typedef struct ColorEntry { > >>> >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const > >>> >> > > char > >>> >> > > *p, int > >>> >> > > len) > >>> >> > > const ColorEntry *entry; > >>> >> > > char color_name[100]; > >>> >> > > > >>> >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > >>> >> > > + > >>> >> > > if (*p == '#') { > >>> >> > > p++; > >>> >> > > len--; > >>> >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext > >>> >> > > *avctx, > >>> >> > > void *data, > >>> >> > > { > >>> >> > > XPMDecContext *x = avctx->priv_data; > >>> >> > > AVFrame *p=data; > >>> >> > > -const uint8_t *end, *ptr = avpkt->data; > >>> >> > > +const uint8_t *end, *ptr; > >>> >> > > int ncolors, cpp, ret, i, j; > >>> >> > > int64_t size; > >>> >> > > uint32_t *dst; > >>> >> > > > >>> >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > >>> >> > > > >>> >> > > -end = avpkt->data + avpkt->size; > >>> >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > >>> >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > >>> >> > > +if (!x->buf) > >>> >> > > +return AVERROR(ENOMEM); > >>> >> > > +memcpy(x->buf, avpkt->data, avpkt->size); > >>> >> > > +x->buf[avpkt->size] = 0; > >>> >> > > + > >>> >> > > +ptr = x->buf; > >>> >> > > +end = x->buf + avpkt->size; > >>> >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > >>> >> > > ptr++; > >>> >> > > > >>> >> > > -if (ptr >= end) { > >>> >> > > +if (end - ptr <= 9) { > >>> >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > >>> >> > > return AVERROR_INVALIDDATA; > >>> >> > > } > >>> >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext > >>> >> > > *avctx, > >>> >> > > void > >>> >> > > *data, > >>> >> > > > >>> >> > > size = 1; > >>> >> > > for (i = 0; i < cpp; i++) > >>> >> > > -size *= 94; > >>> >> > > +size *= 95; > >>> >> > > > >>> >> > > if (ncolors <= 0 || ncolors > size) { > >>> >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: > >>> >> > > %d\n", > >>> >> > > ncolors); > >>> >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext > >>> >> > > *avctx, > >>> >> > > void *data, > >>> >> > > return AVERROR(ENOMEM); > >>> >> > > > >>> >> > > ptr += mod_strcspn(ptr, ",") + 1; > >>> >> > > +if (end - ptr < 1) > >>> >> > > +return AVERROR_INVALIDDATA; > >>> >> > > + > >>> >> > > for (i = 0; i < ncolors; i++) { > >>> >> > > const uint8_t *index; > >>> >> > > int len; > >>> >> > > > >>> >> > > ptr += mod_strcspn(ptr, "\"") + 1; > >>> >> > > -if (ptr + cpp > end) > >>> >> > > +if (end - ptr < cpp) > >>> >> > > return AVERROR_INVALIDDATA; > >>> >> > > index = ptr; > >>> >> > > ptr += cpp; > >>> >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext > >>> >> > > *avctx, > >>> >> > > void *data, > >>> >> > > > >>> >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); > >>> >> > > ptr += mod_strcspn(ptr, ",") + 1; > >>> >> > > +if (end - ptr < 1) > >>> >> > > +return AVERROR_INVALIDDATA; > >>> >> > > } > >>> >> > > > >>>
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Fri, May 12, 2017 at 08:58:52PM +0200, Paul B Mahol wrote: > On 5/12/17, Michael Niedermayerwrote: > > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > >> On 5/12/17, Michael Niedermayer wrote: > >> > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > >> >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > >> >> > On 5/11/17, Michael Niedermayer wrote: > >> >> > > Most of these were found through code review in response to > >> >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > >> >> > > There is thus no testcase for most of this. > >> >> > > The initial issue was Found-by: continuous fuzzing process > >> >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > >> >> > > > >> >> > > Signed-off-by: Michael Niedermayer > >> >> > > --- > >> >> > > libavcodec/xpmdec.c | 37 ++--- > >> >> > > 1 file changed, 30 insertions(+), 7 deletions(-) > >> >> > > > >> >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > >> >> > > index 9112d4cb5e..03172e4aad 100644 > >> >> > > --- a/libavcodec/xpmdec.c > >> >> > > +++ b/libavcodec/xpmdec.c > >> >> > > @@ -29,6 +29,8 @@ > >> >> > > typedef struct XPMContext { > >> >> > > uint32_t *pixels; > >> >> > > intpixels_size; > >> >> > > +uint8_t *buf; > >> >> > > +intbuf_size; > >> >> > > } XPMDecContext; > >> >> > > > >> >> > > typedef struct ColorEntry { > >> >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const > >> >> > > char > >> >> > > *p, int > >> >> > > len) > >> >> > > const ColorEntry *entry; > >> >> > > char color_name[100]; > >> >> > > > >> >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > >> >> > > + > >> >> > > if (*p == '#') { > >> >> > > p++; > >> >> > > len--; > >> >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext > >> >> > > *avctx, > >> >> > > void *data, > >> >> > > { > >> >> > > XPMDecContext *x = avctx->priv_data; > >> >> > > AVFrame *p=data; > >> >> > > -const uint8_t *end, *ptr = avpkt->data; > >> >> > > +const uint8_t *end, *ptr; > >> >> > > int ncolors, cpp, ret, i, j; > >> >> > > int64_t size; > >> >> > > uint32_t *dst; > >> >> > > > >> >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > >> >> > > > >> >> > > -end = avpkt->data + avpkt->size; > >> >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > >> >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > >> >> > > +if (!x->buf) > >> >> > > +return AVERROR(ENOMEM); > >> >> > > +memcpy(x->buf, avpkt->data, avpkt->size); > >> >> > > +x->buf[avpkt->size] = 0; > >> >> > > + > >> >> > > +ptr = x->buf; > >> >> > > +end = x->buf + avpkt->size; > >> >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > >> >> > > ptr++; > >> >> > > > >> >> > > -if (ptr >= end) { > >> >> > > +if (end - ptr <= 9) { > >> >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > >> >> > > return AVERROR_INVALIDDATA; > >> >> > > } > >> >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext > >> >> > > *avctx, > >> >> > > void > >> >> > > *data, > >> >> > > > >> >> > > size = 1; > >> >> > > for (i = 0; i < cpp; i++) > >> >> > > -size *= 94; > >> >> > > +size *= 95; > >> >> > > > >> >> > > if (ncolors <= 0 || ncolors > size) { > >> >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: > >> >> > > %d\n", > >> >> > > ncolors); > >> >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext > >> >> > > *avctx, > >> >> > > void *data, > >> >> > > return AVERROR(ENOMEM); > >> >> > > > >> >> > > ptr += mod_strcspn(ptr, ",") + 1; > >> >> > > +if (end - ptr < 1) > >> >> > > +return AVERROR_INVALIDDATA; > >> >> > > + > >> >> > > for (i = 0; i < ncolors; i++) { > >> >> > > const uint8_t *index; > >> >> > > int len; > >> >> > > > >> >> > > ptr += mod_strcspn(ptr, "\"") + 1; > >> >> > > -if (ptr + cpp > end) > >> >> > > +if (end - ptr < cpp) > >> >> > > return AVERROR_INVALIDDATA; > >> >> > > index = ptr; > >> >> > > ptr += cpp; > >> >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext > >> >> > > *avctx, > >> >> > > void *data, > >> >> > > > >> >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); > >> >> > > ptr += mod_strcspn(ptr, ",") + 1; > >> >> > > +if (end - ptr < 1) > >> >> > > +return AVERROR_INVALIDDATA; > >> >> > > } > >> >> > > > >> >> > > for (i = 0; i < avctx->height; i++) { > >> >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); > >> >> > > +if (end - ptr < 1) > >> >> >
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/12/17, Nicolas Georgewrote: > Le tridi 23 floreal, an CCXXV, Paul B Mahol a ecrit : >> Because I assumed packets are padded with zeroes. > > Because you chose to write quick-and-dirty instead of robust. Thank you. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit : > Because I assumed packets are padded with zeroes. Because you chose to write quick-and-dirty instead of robust. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/12/17, Paul B Maholwrote: > On 5/12/17, Michael Niedermayer wrote: >> On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: >>> On 5/12/17, Michael Niedermayer wrote: >>> > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: >>> >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: >>> >> > On 5/11/17, Michael Niedermayer wrote: >>> >> > > Most of these were found through code review in response to >>> >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 >>> >> > > There is thus no testcase for most of this. >>> >> > > The initial issue was Found-by: continuous fuzzing process >>> >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >>> >> > > >>> >> > > Signed-off-by: Michael Niedermayer >>> >> > > --- >>> >> > > libavcodec/xpmdec.c | 37 ++--- >>> >> > > 1 file changed, 30 insertions(+), 7 deletions(-) >>> >> > > >>> >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c >>> >> > > index 9112d4cb5e..03172e4aad 100644 >>> >> > > --- a/libavcodec/xpmdec.c >>> >> > > +++ b/libavcodec/xpmdec.c >>> >> > > @@ -29,6 +29,8 @@ >>> >> > > typedef struct XPMContext { >>> >> > > uint32_t *pixels; >>> >> > > intpixels_size; >>> >> > > +uint8_t *buf; >>> >> > > +intbuf_size; >>> >> > > } XPMDecContext; >>> >> > > >>> >> > > typedef struct ColorEntry { >>> >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const >>> >> > > char >>> >> > > *p, int >>> >> > > len) >>> >> > > const ColorEntry *entry; >>> >> > > char color_name[100]; >>> >> > > >>> >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); >>> >> > > + >>> >> > > if (*p == '#') { >>> >> > > p++; >>> >> > > len--; >>> >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext >>> >> > > *avctx, >>> >> > > void *data, >>> >> > > { >>> >> > > XPMDecContext *x = avctx->priv_data; >>> >> > > AVFrame *p=data; >>> >> > > -const uint8_t *end, *ptr = avpkt->data; >>> >> > > +const uint8_t *end, *ptr; >>> >> > > int ncolors, cpp, ret, i, j; >>> >> > > int64_t size; >>> >> > > uint32_t *dst; >>> >> > > >>> >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; >>> >> > > >>> >> > > -end = avpkt->data + avpkt->size; >>> >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) >>> >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); >>> >> > > +if (!x->buf) >>> >> > > +return AVERROR(ENOMEM); >>> >> > > +memcpy(x->buf, avpkt->data, avpkt->size); >>> >> > > +x->buf[avpkt->size] = 0; >>> >> > > + >>> >> > > +ptr = x->buf; >>> >> > > +end = x->buf + avpkt->size; >>> >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) >>> >> > > ptr++; >>> >> > > >>> >> > > -if (ptr >= end) { >>> >> > > +if (end - ptr <= 9) { >>> >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); >>> >> > > return AVERROR_INVALIDDATA; >>> >> > > } >>> >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext >>> >> > > *avctx, >>> >> > > void >>> >> > > *data, >>> >> > > >>> >> > > size = 1; >>> >> > > for (i = 0; i < cpp; i++) >>> >> > > -size *= 94; >>> >> > > +size *= 95; >>> >> > > >>> >> > > if (ncolors <= 0 || ncolors > size) { >>> >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: >>> >> > > %d\n", >>> >> > > ncolors); >>> >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext >>> >> > > *avctx, >>> >> > > void *data, >>> >> > > return AVERROR(ENOMEM); >>> >> > > >>> >> > > ptr += mod_strcspn(ptr, ",") + 1; >>> >> > > +if (end - ptr < 1) >>> >> > > +return AVERROR_INVALIDDATA; >>> >> > > + >>> >> > > for (i = 0; i < ncolors; i++) { >>> >> > > const uint8_t *index; >>> >> > > int len; >>> >> > > >>> >> > > ptr += mod_strcspn(ptr, "\"") + 1; >>> >> > > -if (ptr + cpp > end) >>> >> > > +if (end - ptr < cpp) >>> >> > > return AVERROR_INVALIDDATA; >>> >> > > index = ptr; >>> >> > > ptr += cpp; >>> >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext >>> >> > > *avctx, >>> >> > > void *data, >>> >> > > >>> >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); >>> >> > > ptr += mod_strcspn(ptr, ",") + 1; >>> >> > > +if (end - ptr < 1) >>> >> > > +return AVERROR_INVALIDDATA; >>> >> > > } >>> >> > > >>> >> > > for (i = 0; i < avctx->height; i++) { >>> >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); >>> >> > > +if (end - ptr < 1) >>> >> > > +return AVERROR_INVALIDDATA; >>> >> > > ptr += mod_strcspn(ptr, "\"") + 1; >>> >> > > +if (end -
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/12/17, Michael Niedermayerwrote: > On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: >> On 5/12/17, Michael Niedermayer wrote: >> > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: >> >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: >> >> > On 5/11/17, Michael Niedermayer wrote: >> >> > > Most of these were found through code review in response to >> >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 >> >> > > There is thus no testcase for most of this. >> >> > > The initial issue was Found-by: continuous fuzzing process >> >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >> >> > > >> >> > > Signed-off-by: Michael Niedermayer >> >> > > --- >> >> > > libavcodec/xpmdec.c | 37 ++--- >> >> > > 1 file changed, 30 insertions(+), 7 deletions(-) >> >> > > >> >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c >> >> > > index 9112d4cb5e..03172e4aad 100644 >> >> > > --- a/libavcodec/xpmdec.c >> >> > > +++ b/libavcodec/xpmdec.c >> >> > > @@ -29,6 +29,8 @@ >> >> > > typedef struct XPMContext { >> >> > > uint32_t *pixels; >> >> > > intpixels_size; >> >> > > +uint8_t *buf; >> >> > > +intbuf_size; >> >> > > } XPMDecContext; >> >> > > >> >> > > typedef struct ColorEntry { >> >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const >> >> > > char >> >> > > *p, int >> >> > > len) >> >> > > const ColorEntry *entry; >> >> > > char color_name[100]; >> >> > > >> >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); >> >> > > + >> >> > > if (*p == '#') { >> >> > > p++; >> >> > > len--; >> >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext >> >> > > *avctx, >> >> > > void *data, >> >> > > { >> >> > > XPMDecContext *x = avctx->priv_data; >> >> > > AVFrame *p=data; >> >> > > -const uint8_t *end, *ptr = avpkt->data; >> >> > > +const uint8_t *end, *ptr; >> >> > > int ncolors, cpp, ret, i, j; >> >> > > int64_t size; >> >> > > uint32_t *dst; >> >> > > >> >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; >> >> > > >> >> > > -end = avpkt->data + avpkt->size; >> >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) >> >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); >> >> > > +if (!x->buf) >> >> > > +return AVERROR(ENOMEM); >> >> > > +memcpy(x->buf, avpkt->data, avpkt->size); >> >> > > +x->buf[avpkt->size] = 0; >> >> > > + >> >> > > +ptr = x->buf; >> >> > > +end = x->buf + avpkt->size; >> >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) >> >> > > ptr++; >> >> > > >> >> > > -if (ptr >= end) { >> >> > > +if (end - ptr <= 9) { >> >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); >> >> > > return AVERROR_INVALIDDATA; >> >> > > } >> >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext >> >> > > *avctx, >> >> > > void >> >> > > *data, >> >> > > >> >> > > size = 1; >> >> > > for (i = 0; i < cpp; i++) >> >> > > -size *= 94; >> >> > > +size *= 95; >> >> > > >> >> > > if (ncolors <= 0 || ncolors > size) { >> >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: >> >> > > %d\n", >> >> > > ncolors); >> >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext >> >> > > *avctx, >> >> > > void *data, >> >> > > return AVERROR(ENOMEM); >> >> > > >> >> > > ptr += mod_strcspn(ptr, ",") + 1; >> >> > > +if (end - ptr < 1) >> >> > > +return AVERROR_INVALIDDATA; >> >> > > + >> >> > > for (i = 0; i < ncolors; i++) { >> >> > > const uint8_t *index; >> >> > > int len; >> >> > > >> >> > > ptr += mod_strcspn(ptr, "\"") + 1; >> >> > > -if (ptr + cpp > end) >> >> > > +if (end - ptr < cpp) >> >> > > return AVERROR_INVALIDDATA; >> >> > > index = ptr; >> >> > > ptr += cpp; >> >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext >> >> > > *avctx, >> >> > > void *data, >> >> > > >> >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); >> >> > > ptr += mod_strcspn(ptr, ",") + 1; >> >> > > +if (end - ptr < 1) >> >> > > +return AVERROR_INVALIDDATA; >> >> > > } >> >> > > >> >> > > for (i = 0; i < avctx->height; i++) { >> >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); >> >> > > +if (end - ptr < 1) >> >> > > +return AVERROR_INVALIDDATA; >> >> > > ptr += mod_strcspn(ptr, "\"") + 1; >> >> > > +if (end - ptr < 1) >> >> > > +return AVERROR_INVALIDDATA; >> >> > > >> >> > > for (j = 0; j < avctx->width; j++) { >> >> > > -if (ptr + cpp > end) >> >> >
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Fri, May 12, 2017 at 03:29:52PM +0200, Paul B Mahol wrote: > On 5/12/17, Michael Niedermayerwrote: > > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > >> > On 5/11/17, Michael Niedermayer wrote: > >> > > Most of these were found through code review in response to > >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > >> > > There is thus no testcase for most of this. > >> > > The initial issue was Found-by: continuous fuzzing process > >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > >> > > > >> > > Signed-off-by: Michael Niedermayer > >> > > --- > >> > > libavcodec/xpmdec.c | 37 ++--- > >> > > 1 file changed, 30 insertions(+), 7 deletions(-) > >> > > > >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > >> > > index 9112d4cb5e..03172e4aad 100644 > >> > > --- a/libavcodec/xpmdec.c > >> > > +++ b/libavcodec/xpmdec.c > >> > > @@ -29,6 +29,8 @@ > >> > > typedef struct XPMContext { > >> > > uint32_t *pixels; > >> > > intpixels_size; > >> > > +uint8_t *buf; > >> > > +intbuf_size; > >> > > } XPMDecContext; > >> > > > >> > > typedef struct ColorEntry { > >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char > >> > > *p, int > >> > > len) > >> > > const ColorEntry *entry; > >> > > char color_name[100]; > >> > > > >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > >> > > + > >> > > if (*p == '#') { > >> > > p++; > >> > > len--; > >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext > >> > > *avctx, > >> > > void *data, > >> > > { > >> > > XPMDecContext *x = avctx->priv_data; > >> > > AVFrame *p=data; > >> > > -const uint8_t *end, *ptr = avpkt->data; > >> > > +const uint8_t *end, *ptr; > >> > > int ncolors, cpp, ret, i, j; > >> > > int64_t size; > >> > > uint32_t *dst; > >> > > > >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > >> > > > >> > > -end = avpkt->data + avpkt->size; > >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > >> > > +if (!x->buf) > >> > > +return AVERROR(ENOMEM); > >> > > +memcpy(x->buf, avpkt->data, avpkt->size); > >> > > +x->buf[avpkt->size] = 0; > >> > > + > >> > > +ptr = x->buf; > >> > > +end = x->buf + avpkt->size; > >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > >> > > ptr++; > >> > > > >> > > -if (ptr >= end) { > >> > > +if (end - ptr <= 9) { > >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > >> > > return AVERROR_INVALIDDATA; > >> > > } > >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx, > >> > > void > >> > > *data, > >> > > > >> > > size = 1; > >> > > for (i = 0; i < cpp; i++) > >> > > -size *= 94; > >> > > +size *= 95; > >> > > > >> > > if (ncolors <= 0 || ncolors > size) { > >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: > >> > > %d\n", > >> > > ncolors); > >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext > >> > > *avctx, > >> > > void *data, > >> > > return AVERROR(ENOMEM); > >> > > > >> > > ptr += mod_strcspn(ptr, ",") + 1; > >> > > +if (end - ptr < 1) > >> > > +return AVERROR_INVALIDDATA; > >> > > + > >> > > for (i = 0; i < ncolors; i++) { > >> > > const uint8_t *index; > >> > > int len; > >> > > > >> > > ptr += mod_strcspn(ptr, "\"") + 1; > >> > > -if (ptr + cpp > end) > >> > > +if (end - ptr < cpp) > >> > > return AVERROR_INVALIDDATA; > >> > > index = ptr; > >> > > ptr += cpp; > >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext > >> > > *avctx, > >> > > void *data, > >> > > > >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); > >> > > ptr += mod_strcspn(ptr, ",") + 1; > >> > > +if (end - ptr < 1) > >> > > +return AVERROR_INVALIDDATA; > >> > > } > >> > > > >> > > for (i = 0; i < avctx->height; i++) { > >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); > >> > > +if (end - ptr < 1) > >> > > +return AVERROR_INVALIDDATA; > >> > > ptr += mod_strcspn(ptr, "\"") + 1; > >> > > +if (end - ptr < 1) > >> > > +return AVERROR_INVALIDDATA; > >> > > > >> > > for (j = 0; j < avctx->width; j++) { > >> > > -if (ptr + cpp > end) > >> > > +if (end - ptr < cpp) > >> > > return AVERROR_INVALIDDATA; > >> > > > >> > > if ((ret = ascii2index(ptr, cpp)) < 0) > >> > > @@ -405,6 +425,9 @@ static av_cold int >
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/12/17, Nicolas Georgewrote: > Le tridi 23 floreal, an CCXXV, Paul B Mahol a ecrit : >> I told you that its unacceptable. > > And I told you, before you pushed, that your cowboy-style parser was > unacceptable. You chose to disregard it. Because I assumed packets are padded with zeroes. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
Le tridi 23 floréal, an CCXXV, Paul B Mahol a écrit : > I told you that its unacceptable. And I told you, before you pushed, that your cowboy-style parser was unacceptable. You chose to disregard it. Regards, -- Nicolas George ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/12/17, Michael Niedermayerwrote: > On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: >> On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: >> > On 5/11/17, Michael Niedermayer wrote: >> > > Most of these were found through code review in response to >> > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 >> > > There is thus no testcase for most of this. >> > > The initial issue was Found-by: continuous fuzzing process >> > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg >> > > >> > > Signed-off-by: Michael Niedermayer >> > > --- >> > > libavcodec/xpmdec.c | 37 ++--- >> > > 1 file changed, 30 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c >> > > index 9112d4cb5e..03172e4aad 100644 >> > > --- a/libavcodec/xpmdec.c >> > > +++ b/libavcodec/xpmdec.c >> > > @@ -29,6 +29,8 @@ >> > > typedef struct XPMContext { >> > > uint32_t *pixels; >> > > intpixels_size; >> > > +uint8_t *buf; >> > > +intbuf_size; >> > > } XPMDecContext; >> > > >> > > typedef struct ColorEntry { >> > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char >> > > *p, int >> > > len) >> > > const ColorEntry *entry; >> > > char color_name[100]; >> > > >> > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); >> > > + >> > > if (*p == '#') { >> > > p++; >> > > len--; >> > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext >> > > *avctx, >> > > void *data, >> > > { >> > > XPMDecContext *x = avctx->priv_data; >> > > AVFrame *p=data; >> > > -const uint8_t *end, *ptr = avpkt->data; >> > > +const uint8_t *end, *ptr; >> > > int ncolors, cpp, ret, i, j; >> > > int64_t size; >> > > uint32_t *dst; >> > > >> > > avctx->pix_fmt = AV_PIX_FMT_BGRA; >> > > >> > > -end = avpkt->data + avpkt->size; >> > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) >> > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); >> > > +if (!x->buf) >> > > +return AVERROR(ENOMEM); >> > > +memcpy(x->buf, avpkt->data, avpkt->size); >> > > +x->buf[avpkt->size] = 0; >> > > + >> > > +ptr = x->buf; >> > > +end = x->buf + avpkt->size; >> > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) >> > > ptr++; >> > > >> > > -if (ptr >= end) { >> > > +if (end - ptr <= 9) { >> > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); >> > > return AVERROR_INVALIDDATA; >> > > } >> > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx, >> > > void >> > > *data, >> > > >> > > size = 1; >> > > for (i = 0; i < cpp; i++) >> > > -size *= 94; >> > > +size *= 95; >> > > >> > > if (ncolors <= 0 || ncolors > size) { >> > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: >> > > %d\n", >> > > ncolors); >> > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext >> > > *avctx, >> > > void *data, >> > > return AVERROR(ENOMEM); >> > > >> > > ptr += mod_strcspn(ptr, ",") + 1; >> > > +if (end - ptr < 1) >> > > +return AVERROR_INVALIDDATA; >> > > + >> > > for (i = 0; i < ncolors; i++) { >> > > const uint8_t *index; >> > > int len; >> > > >> > > ptr += mod_strcspn(ptr, "\"") + 1; >> > > -if (ptr + cpp > end) >> > > +if (end - ptr < cpp) >> > > return AVERROR_INVALIDDATA; >> > > index = ptr; >> > > ptr += cpp; >> > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext >> > > *avctx, >> > > void *data, >> > > >> > > x->pixels[ret] = color_string_to_rgba(ptr, len); >> > > ptr += mod_strcspn(ptr, ",") + 1; >> > > +if (end - ptr < 1) >> > > +return AVERROR_INVALIDDATA; >> > > } >> > > >> > > for (i = 0; i < avctx->height; i++) { >> > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); >> > > +if (end - ptr < 1) >> > > +return AVERROR_INVALIDDATA; >> > > ptr += mod_strcspn(ptr, "\"") + 1; >> > > +if (end - ptr < 1) >> > > +return AVERROR_INVALIDDATA; >> > > >> > > for (j = 0; j < avctx->width; j++) { >> > > -if (ptr + cpp > end) >> > > +if (end - ptr < cpp) >> > > return AVERROR_INVALIDDATA; >> > > >> > > if ((ret = ascii2index(ptr, cpp)) < 0) >> > > @@ -405,6 +425,9 @@ static av_cold int >> > > xpm_decode_close(AVCodecContext >> > > *avctx) >> > > XPMDecContext *x = avctx->priv_data; >> > > av_freep(>pixels); >> > > >> > > +av_freep(>buf); >> > > +x->buf_size = 0; >> > > + >> > > return 0; >> > > } >> > > >> > > -- >> > > 2.11.0 >> > > >> > >
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On Thu, May 11, 2017 at 11:17:33AM +0200, Michael Niedermayer wrote: > On Thu, May 11, 2017 at 09:01:57AM +0200, Paul B Mahol wrote: > > On 5/11/17, Michael Niedermayerwrote: > > > Most of these were found through code review in response to > > > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > > > There is thus no testcase for most of this. > > > The initial issue was Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer > > > --- > > > libavcodec/xpmdec.c | 37 ++--- > > > 1 file changed, 30 insertions(+), 7 deletions(-) > > > > > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > > > index 9112d4cb5e..03172e4aad 100644 > > > --- a/libavcodec/xpmdec.c > > > +++ b/libavcodec/xpmdec.c > > > @@ -29,6 +29,8 @@ > > > typedef struct XPMContext { > > > uint32_t *pixels; > > > intpixels_size; > > > +uint8_t *buf; > > > +intbuf_size; > > > } XPMDecContext; > > > > > > typedef struct ColorEntry { > > > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char *p, > > > int > > > len) > > > const ColorEntry *entry; > > > char color_name[100]; > > > > > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > > > + > > > if (*p == '#') { > > > p++; > > > len--; > > > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext *avctx, > > > void *data, > > > { > > > XPMDecContext *x = avctx->priv_data; > > > AVFrame *p=data; > > > -const uint8_t *end, *ptr = avpkt->data; > > > +const uint8_t *end, *ptr; > > > int ncolors, cpp, ret, i, j; > > > int64_t size; > > > uint32_t *dst; > > > > > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > > > > > > -end = avpkt->data + avpkt->size; > > > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > > > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > > > +if (!x->buf) > > > +return AVERROR(ENOMEM); > > > +memcpy(x->buf, avpkt->data, avpkt->size); > > > +x->buf[avpkt->size] = 0; > > > + > > > +ptr = x->buf; > > > +end = x->buf + avpkt->size; > > > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > > > ptr++; > > > > > > -if (ptr >= end) { > > > +if (end - ptr <= 9) { > > > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > > > return AVERROR_INVALIDDATA; > > > } > > > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx, > > > void > > > *data, > > > > > > size = 1; > > > for (i = 0; i < cpp; i++) > > > -size *= 94; > > > +size *= 95; > > > > > > if (ncolors <= 0 || ncolors > size) { > > > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: %d\n", > > > ncolors); > > > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext *avctx, > > > void *data, > > > return AVERROR(ENOMEM); > > > > > > ptr += mod_strcspn(ptr, ",") + 1; > > > +if (end - ptr < 1) > > > +return AVERROR_INVALIDDATA; > > > + > > > for (i = 0; i < ncolors; i++) { > > > const uint8_t *index; > > > int len; > > > > > > ptr += mod_strcspn(ptr, "\"") + 1; > > > -if (ptr + cpp > end) > > > +if (end - ptr < cpp) > > > return AVERROR_INVALIDDATA; > > > index = ptr; > > > ptr += cpp; > > > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext *avctx, > > > void *data, > > > > > > x->pixels[ret] = color_string_to_rgba(ptr, len); > > > ptr += mod_strcspn(ptr, ",") + 1; > > > +if (end - ptr < 1) > > > +return AVERROR_INVALIDDATA; > > > } > > > > > > for (i = 0; i < avctx->height; i++) { > > > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); > > > +if (end - ptr < 1) > > > +return AVERROR_INVALIDDATA; > > > ptr += mod_strcspn(ptr, "\"") + 1; > > > +if (end - ptr < 1) > > > +return AVERROR_INVALIDDATA; > > > > > > for (j = 0; j < avctx->width; j++) { > > > -if (ptr + cpp > end) > > > +if (end - ptr < cpp) > > > return AVERROR_INVALIDDATA; > > > > > > if ((ret = ascii2index(ptr, cpp)) < 0) > > > @@ -405,6 +425,9 @@ static av_cold int xpm_decode_close(AVCodecContext > > > *avctx) > > > XPMDecContext *x = avctx->priv_data; > > > av_freep(>pixels); > > > > > > +av_freep(>buf); > > > +x->buf_size = 0; > > > + > > > return 0; > > > } > > > > > > -- > > > 2.11.0 > > > > > > ___ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > This is suboptimal solution. > > That comment isnt helpfull in improving the patch. It
Re: [FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
On 5/11/17, Michael Niedermayerwrote: > Most of these were found through code review in response to > fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 > There is thus no testcase for most of this. > The initial issue was Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > Signed-off-by: Michael Niedermayer > --- > libavcodec/xpmdec.c | 37 ++--- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c > index 9112d4cb5e..03172e4aad 100644 > --- a/libavcodec/xpmdec.c > +++ b/libavcodec/xpmdec.c > @@ -29,6 +29,8 @@ > typedef struct XPMContext { > uint32_t *pixels; > intpixels_size; > +uint8_t *buf; > +intbuf_size; > } XPMDecContext; > > typedef struct ColorEntry { > @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char *p, int > len) > const ColorEntry *entry; > char color_name[100]; > > +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); > + > if (*p == '#') { > p++; > len--; > @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext *avctx, > void *data, > { > XPMDecContext *x = avctx->priv_data; > AVFrame *p=data; > -const uint8_t *end, *ptr = avpkt->data; > +const uint8_t *end, *ptr; > int ncolors, cpp, ret, i, j; > int64_t size; > uint32_t *dst; > > avctx->pix_fmt = AV_PIX_FMT_BGRA; > > -end = avpkt->data + avpkt->size; > -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) > +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); > +if (!x->buf) > +return AVERROR(ENOMEM); > +memcpy(x->buf, avpkt->data, avpkt->size); > +x->buf[avpkt->size] = 0; > + > +ptr = x->buf; > +end = x->buf + avpkt->size; > +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) > ptr++; > > -if (ptr >= end) { > +if (end - ptr <= 9) { > av_log(avctx, AV_LOG_ERROR, "missing signature\n"); > return AVERROR_INVALIDDATA; > } > @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx, void > *data, > > size = 1; > for (i = 0; i < cpp; i++) > -size *= 94; > +size *= 95; > > if (ncolors <= 0 || ncolors > size) { > av_log(avctx, AV_LOG_ERROR, "invalid number of colors: %d\n", > ncolors); > @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext *avctx, > void *data, > return AVERROR(ENOMEM); > > ptr += mod_strcspn(ptr, ",") + 1; > +if (end - ptr < 1) > +return AVERROR_INVALIDDATA; > + > for (i = 0; i < ncolors; i++) { > const uint8_t *index; > int len; > > ptr += mod_strcspn(ptr, "\"") + 1; > -if (ptr + cpp > end) > +if (end - ptr < cpp) > return AVERROR_INVALIDDATA; > index = ptr; > ptr += cpp; > @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext *avctx, > void *data, > > x->pixels[ret] = color_string_to_rgba(ptr, len); > ptr += mod_strcspn(ptr, ",") + 1; > +if (end - ptr < 1) > +return AVERROR_INVALIDDATA; > } > > for (i = 0; i < avctx->height; i++) { > dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); > +if (end - ptr < 1) > +return AVERROR_INVALIDDATA; > ptr += mod_strcspn(ptr, "\"") + 1; > +if (end - ptr < 1) > +return AVERROR_INVALIDDATA; > > for (j = 0; j < avctx->width; j++) { > -if (ptr + cpp > end) > +if (end - ptr < cpp) > return AVERROR_INVALIDDATA; > > if ((ret = ascii2index(ptr, cpp)) < 0) > @@ -405,6 +425,9 @@ static av_cold int xpm_decode_close(AVCodecContext > *avctx) > XPMDecContext *x = avctx->priv_data; > av_freep(>pixels); > > +av_freep(>buf); > +x->buf_size = 0; > + > return 0; > } > > -- > 2.11.0 > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > This is suboptimal solution. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/xpmdec: Fix multiple pointer/memory issues
Most of these were found through code review in response to fixing 1466/clusterfuzz-testcase-minimized-5961584419536896 There is thus no testcase for most of this. The initial issue was Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg Signed-off-by: Michael Niedermayer--- libavcodec/xpmdec.c | 37 ++--- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c index 9112d4cb5e..03172e4aad 100644 --- a/libavcodec/xpmdec.c +++ b/libavcodec/xpmdec.c @@ -29,6 +29,8 @@ typedef struct XPMContext { uint32_t *pixels; intpixels_size; +uint8_t *buf; +intbuf_size; } XPMDecContext; typedef struct ColorEntry { @@ -233,6 +235,8 @@ static uint32_t color_string_to_rgba(const char *p, int len) const ColorEntry *entry; char color_name[100]; +len = FFMIN(FFMAX(len, 0), sizeof(color_name) - 1); + if (*p == '#') { p++; len--; @@ -299,18 +303,25 @@ static int xpm_decode_frame(AVCodecContext *avctx, void *data, { XPMDecContext *x = avctx->priv_data; AVFrame *p=data; -const uint8_t *end, *ptr = avpkt->data; +const uint8_t *end, *ptr; int ncolors, cpp, ret, i, j; int64_t size; uint32_t *dst; avctx->pix_fmt = AV_PIX_FMT_BGRA; -end = avpkt->data + avpkt->size; -while (memcmp(ptr, "/* XPM */", 9) && ptr < end - 9) +av_fast_padded_malloc(>buf, >buf_size, avpkt->size); +if (!x->buf) +return AVERROR(ENOMEM); +memcpy(x->buf, avpkt->data, avpkt->size); +x->buf[avpkt->size] = 0; + +ptr = x->buf; +end = x->buf + avpkt->size; +while (end - ptr > 9 && memcmp(ptr, "/* XPM */", 9)) ptr++; -if (ptr >= end) { +if (end - ptr <= 9) { av_log(avctx, AV_LOG_ERROR, "missing signature\n"); return AVERROR_INVALIDDATA; } @@ -335,7 +346,7 @@ static int xpm_decode_frame(AVCodecContext *avctx, void *data, size = 1; for (i = 0; i < cpp; i++) -size *= 94; +size *= 95; if (ncolors <= 0 || ncolors > size) { av_log(avctx, AV_LOG_ERROR, "invalid number of colors: %d\n", ncolors); @@ -349,12 +360,15 @@ static int xpm_decode_frame(AVCodecContext *avctx, void *data, return AVERROR(ENOMEM); ptr += mod_strcspn(ptr, ",") + 1; +if (end - ptr < 1) +return AVERROR_INVALIDDATA; + for (i = 0; i < ncolors; i++) { const uint8_t *index; int len; ptr += mod_strcspn(ptr, "\"") + 1; -if (ptr + cpp > end) +if (end - ptr < cpp) return AVERROR_INVALIDDATA; index = ptr; ptr += cpp; @@ -373,14 +387,20 @@ static int xpm_decode_frame(AVCodecContext *avctx, void *data, x->pixels[ret] = color_string_to_rgba(ptr, len); ptr += mod_strcspn(ptr, ",") + 1; +if (end - ptr < 1) +return AVERROR_INVALIDDATA; } for (i = 0; i < avctx->height; i++) { dst = (uint32_t *)(p->data[0] + i * p->linesize[0]); +if (end - ptr < 1) +return AVERROR_INVALIDDATA; ptr += mod_strcspn(ptr, "\"") + 1; +if (end - ptr < 1) +return AVERROR_INVALIDDATA; for (j = 0; j < avctx->width; j++) { -if (ptr + cpp > end) +if (end - ptr < cpp) return AVERROR_INVALIDDATA; if ((ret = ascii2index(ptr, cpp)) < 0) @@ -405,6 +425,9 @@ static av_cold int xpm_decode_close(AVCodecContext *avctx) XPMDecContext *x = avctx->priv_data; av_freep(>pixels); +av_freep(>buf); +x->buf_size = 0; + return 0; } -- 2.11.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel