| Message ID | 0056a93a347829e72cd6d09d48062978ca4ac6e0.1651916204.git.ffmpegagent@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | libavformat/asf: fix handling of byte array length values | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sat, May 07, 2022 at 09:36:34AM +0000, softworkz wrote: > From: softworkz <softworkz@hotmail.com> > > The spec allows attachment sizes of up to UINT32_MAX while > we can handle only sizes up to INT32_MAX (in downstream > code) > > The debug.assert in get_tag didn't really address this, > and truncating the value_len in calling methods cannot > be used because the length value is required in order to > continue parsing. This adds a check with log message in > ff_asf_handle_byte_array to handle those (rare) cases. > > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > libavformat/asf.c | 12 +++++++++--- > libavformat/asf.h | 2 +- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/libavformat/asf.c b/libavformat/asf.c > index 1ac8b5f078..179b66a2b4 100644 > --- a/libavformat/asf.c > +++ b/libavformat/asf.c > @@ -267,12 +267,18 @@ static int get_id3_tag(AVFormatContext *s, int len) > } > > int ff_asf_handle_byte_array(AVFormatContext *s, const char *name, > - int val_len) > + uint32_t val_len) > { > + if (val_len > INT32_MAX) { > + av_log(s, AV_LOG_VERBOSE, "Unable to handle byte arrays > INT32_MAX in tag %s.\n", name); > + return 1; > + } > + > if (!strcmp(name, "WM/Picture")) // handle cover art > - return asf_read_picture(s, val_len); > + return asf_read_picture(s, (int)val_len); > else if (!strcmp(name, "ID3")) // handle ID3 tag > - return get_id3_tag(s, val_len); > + return get_id3_tag(s, (int)val_len); unneeded > > + av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag %s.\n", name); Probably this should be DEBUG thx [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Michael Niedermayer > Sent: Saturday, May 7, 2022 8:49 PM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2 01/11] libavformat/asf: fix > handling of byte array length values > > On Sat, May 07, 2022 at 09:36:34AM +0000, softworkz wrote: > > From: softworkz <softworkz@hotmail.com> > > > > The spec allows attachment sizes of up to UINT32_MAX while > > we can handle only sizes up to INT32_MAX (in downstream > > code) > > > > The debug.assert in get_tag didn't really address this, > > and truncating the value_len in calling methods cannot > > be used because the length value is required in order to > > continue parsing. This adds a check with log message in > > ff_asf_handle_byte_array to handle those (rare) cases. > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > --- > > libavformat/asf.c | 12 +++++++++--- > > libavformat/asf.h | 2 +- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/libavformat/asf.c b/libavformat/asf.c > > index 1ac8b5f078..179b66a2b4 100644 > > --- a/libavformat/asf.c > > +++ b/libavformat/asf.c > > @@ -267,12 +267,18 @@ static int get_id3_tag(AVFormatContext *s, int > len) > > } > > > > int ff_asf_handle_byte_array(AVFormatContext *s, const char *name, > > - int val_len) > > + uint32_t val_len) > > { > > + if (val_len > INT32_MAX) { > > + av_log(s, AV_LOG_VERBOSE, "Unable to handle byte arrays > > INT32_MAX in tag %s.\n", name); > > + return 1; > > + } > > + > > > if (!strcmp(name, "WM/Picture")) // handle cover art > > - return asf_read_picture(s, val_len); > > + return asf_read_picture(s, (int)val_len); > > else if (!strcmp(name, "ID3")) // handle ID3 tag > > - return get_id3_tag(s, val_len); > > + return get_id3_tag(s, (int)val_len); > > unneeded Hi Michael, thanks a lot for reviewing! I think we had talked about this a while ago. From my point of view, that explicit cast to int, tells me, every other reader of the code as well as any static analysis or linting tools that the developer has been aware of the data type mismatch between supplied variable and the parameter type and that the conversion is intended rather than accidental. But I don't want to insist - I have removed it. > > > > + av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag > %s.\n", name); > > Probably this should be DEBUG The problem with DEBUG is that some components are spitting out so many lines with log level DEBUG that you'd hardly ever see it unless you'd explicitly search for that exact line, while that line is rather of the kind that you don't expect it. I've changed it to DEBUG, though. > > if (!strcmp(name, "AspectRatioX")){ > > - int aspect_x = get_value(s->pb, value_type, 16); > > + const uint64_t aspect_x = get_value(s->pb, value_type, > 16); > > + if (aspect_x > INT32_MAX) { > > + av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX > value: %"PRIu64"\n", aspect_x); > > + return AVERROR(ENOTSUP); > > + } > > if(stream_num < 128) > > - asf->dar[stream_num].num = aspect_x; > > + asf->dar[stream_num].num = (int)aspect_x; > > } else if(!strcmp(name, "AspectRatioY")){ > > - int aspect_y = get_value(s->pb, value_type, 16); > > + const uint64_t aspect_y = get_value(s->pb, value_type, > 16); > > + if (aspect_y > INT32_MAX) { > > + av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY > value: %"PRIu64"\n", aspect_y); > > + return AVERROR(ENOTSUP); > > + } > > if(stream_num < 128) > > - asf->dar[stream_num].den = aspect_y; > > + asf->dar[stream_num].den = (int)aspect_y; > > } else { > > If you go to the length to do something with oddly huge aspect > components > maybe change dar to 2 uint64_t and check it in one place instead of 2 > also the av_reduce() can handle a wider range than int32 Good idea, didn't know that av_reduce() can take larger numbers. Done. Thanks again, softworkz
diff --git a/libavformat/asf.c b/libavformat/asf.c index 1ac8b5f078..179b66a2b4 100644 --- a/libavformat/asf.c +++ b/libavformat/asf.c @@ -267,12 +267,18 @@ static int get_id3_tag(AVFormatContext *s, int len) } int ff_asf_handle_byte_array(AVFormatContext *s, const char *name, - int val_len) + uint32_t val_len) { + if (val_len > INT32_MAX) { + av_log(s, AV_LOG_VERBOSE, "Unable to handle byte arrays > INT32_MAX in tag %s.\n", name); + return 1; + } + if (!strcmp(name, "WM/Picture")) // handle cover art - return asf_read_picture(s, val_len); + return asf_read_picture(s, (int)val_len); else if (!strcmp(name, "ID3")) // handle ID3 tag - return get_id3_tag(s, val_len); + return get_id3_tag(s, (int)val_len); + av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag %s.\n", name); return 1; } diff --git a/libavformat/asf.h b/libavformat/asf.h index 01cc4f7a46..4d28560f56 100644 --- a/libavformat/asf.h +++ b/libavformat/asf.h @@ -111,7 +111,7 @@ extern const AVMetadataConv ff_asf_metadata_conv[]; * is unsupported by this function and 0 otherwise. */ int ff_asf_handle_byte_array(AVFormatContext *s, const char *name, - int val_len); + uint32_t val_len); #define ASF_PACKET_FLAG_ERROR_CORRECTION_PRESENT 0x80 //1000 0000