| Message ID | MN2PR04MB5981E0EFD72EE723973B93A9BAAA9@MN2PR04MB5981.namprd04.prod.outlook.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [FFmpeg-devel,v5,1/7] libavformat/asf: Fix handling of byte array length values | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > get_value had a return type of int, which means that reading > QWORDS (case 4) was broken due to truncation of the result from > avio_rl64(). > > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > v5: Split into pieces as requested > > libavformat/asfdec_f.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > index 72ba8b32a0..076b5ab147 100644 > --- a/libavformat/asfdec_f.c > +++ b/libavformat/asfdec_f.c > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData *pd) > > /* size of type 2 (BOOL) is 32bit for "Extended Content Description Object" > * but 16 bit for "Metadata Object" and "Metadata Library Object" */ > -static int get_value(AVIOContext *pb, int type, int type2_size) > +static uint64_t get_value(AVIOContext *pb, int type, int type2_size) > { > switch (type) { > case 2: > @@ -568,9 +568,9 @@ static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > * ASF stream count starts at 1. I am using 0 to the container value > * since it's unused. */ > if (!strcmp(name, "AspectRatioX")) > - asf->dar[0].num = get_value(s->pb, value_type, 32); > + asf->dar[0].num = (int)get_value(s->pb, value_type, 32); > else if (!strcmp(name, "AspectRatioY")) > - asf->dar[0].den = get_value(s->pb, value_type, 32); > + asf->dar[0].den = (int)get_value(s->pb, value_type, 32); > else > get_tag(s, name, value_type, value_len, 32); > } > @@ -630,11 +630,11 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size) > i, stream_num, name_len_utf16, value_type, value_len, name); > > if (!strcmp(name, "AspectRatioX")){ > - int aspect_x = get_value(s->pb, value_type, 16); > + int aspect_x = (int)get_value(s->pb, value_type, 16); > if(stream_num < 128) > asf->dar[stream_num].num = aspect_x; > } else if(!strcmp(name, "AspectRatioY")){ > - int aspect_y = get_value(s->pb, value_type, 16); > + int aspect_y = (int)get_value(s->pb, value_type, 16); > if(stream_num < 128) > asf->dar[stream_num].den = aspect_y; > } else { a 64bit/64bit aspect does not work after this, it still just silently truncates the value to 32bit, the truncation just happens later thx [...]
On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer wrote: > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > get_value had a return type of int, which means that reading > > QWORDS (case 4) was broken due to truncation of the result from > > avio_rl64(). > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > --- > > v5: Split into pieces as requested > > > > libavformat/asfdec_f.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > index 72ba8b32a0..076b5ab147 100644 > > --- a/libavformat/asfdec_f.c > > +++ b/libavformat/asfdec_f.c > > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData *pd) > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content Description Object" > > * but 16 bit for "Metadata Object" and "Metadata Library Object" */ > > -static int get_value(AVIOContext *pb, int type, int type2_size) > > +static uint64_t get_value(AVIOContext *pb, int type, int type2_size) > > { > > switch (type) { > > case 2: > > @@ -568,9 +568,9 @@ static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > * ASF stream count starts at 1. I am using 0 to the container value > > * since it's unused. */ > > if (!strcmp(name, "AspectRatioX")) > > - asf->dar[0].num = get_value(s->pb, value_type, 32); > > + asf->dar[0].num = (int)get_value(s->pb, value_type, 32); > > else if (!strcmp(name, "AspectRatioY")) > > - asf->dar[0].den = get_value(s->pb, value_type, 32); > > + asf->dar[0].den = (int)get_value(s->pb, value_type, 32); > > else > > get_tag(s, name, value_type, value_len, 32); > > } > > @@ -630,11 +630,11 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size) > > i, stream_num, name_len_utf16, value_type, value_len, name); > > > > if (!strcmp(name, "AspectRatioX")){ > > - int aspect_x = get_value(s->pb, value_type, 16); > > + int aspect_x = (int)get_value(s->pb, value_type, 16); > > if(stream_num < 128) > > asf->dar[stream_num].num = aspect_x; > > } else if(!strcmp(name, "AspectRatioY")){ > > - int aspect_y = get_value(s->pb, value_type, 16); > > + int aspect_y = (int)get_value(s->pb, value_type, 16); > > if(stream_num < 128) > > asf->dar[stream_num].den = aspect_y; > > } else { > > a 64bit/64bit aspect does not work after this, it still just silently > truncates the value to 32bit, the truncation just happens later forgot: see av_reduce() [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Michael Niedermayer > Sent: Thursday, 30 September 2021 14:04 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix > get_value return type > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer wrote: > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > > get_value had a return type of int, which means that reading > > > QWORDS (case 4) was broken due to truncation of the result from > > > avio_rl64(). > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > > --- > > > v5: Split into pieces as requested > > > > > > libavformat/asfdec_f.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > index 72ba8b32a0..076b5ab147 100644 > > > --- a/libavformat/asfdec_f.c > > > +++ b/libavformat/asfdec_f.c > > > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData *pd) > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content > Description Object" > > > * but 16 bit for "Metadata Object" and "Metadata Library > Object" */ > > > -static int get_value(AVIOContext *pb, int type, int type2_size) > > > +static uint64_t get_value(AVIOContext *pb, int type, int > type2_size) > > > { > > > switch (type) { > > > case 2: > > > @@ -568,9 +568,9 @@ static int > asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > > * ASF stream count starts at 1. I am using 0 to the > container value > > > * since it's unused. */ > > > if (!strcmp(name, "AspectRatioX")) > > > - asf->dar[0].num = get_value(s->pb, value_type, 32); > > > + asf->dar[0].num = (int)get_value(s->pb, value_type, > 32); > > > else if (!strcmp(name, "AspectRatioY")) > > > - asf->dar[0].den = get_value(s->pb, value_type, 32); > > > + asf->dar[0].den = (int)get_value(s->pb, value_type, > 32); > > > else > > > get_tag(s, name, value_type, value_len, 32); > > > } > > > @@ -630,11 +630,11 @@ static int > asf_read_metadata(AVFormatContext *s, int64_t size) > > > i, stream_num, name_len_utf16, value_type, > value_len, name); > > > > > > if (!strcmp(name, "AspectRatioX")){ > > > - int aspect_x = get_value(s->pb, value_type, 16); > > > + int aspect_x = (int)get_value(s->pb, value_type, > 16); > > > if(stream_num < 128) > > > asf->dar[stream_num].num = aspect_x; > > > } else if(!strcmp(name, "AspectRatioY")){ > > > - int aspect_y = get_value(s->pb, value_type, 16); > > > + int aspect_y = (int)get_value(s->pb, value_type, > 16); > > > if(stream_num < 128) > > > asf->dar[stream_num].den = aspect_y; > > > } else { > > > > a 64bit/64bit aspect does not work after this, it still just > silently > > truncates the value to 32bit, the truncation just happens later > > forgot: see av_reduce() These lines are not about making any change. The get_value() calls for AspectRatioX/Y are returning 16bit values, before and after. I added the explicit cast to (int) because get_value() doesn't return int anymore and as such, to indicate that the cast is intentional. But the values in this case are limited to 16bit integers anyway. softworkz
On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote: > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Michael Niedermayer > > Sent: Thursday, 30 September 2021 14:04 > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix > > get_value return type > > > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer wrote: > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > > > get_value had a return type of int, which means that reading > > > > QWORDS (case 4) was broken due to truncation of the result from > > > > avio_rl64(). > > > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > > > --- > > > > v5: Split into pieces as requested > > > > > > > > libavformat/asfdec_f.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > > index 72ba8b32a0..076b5ab147 100644 > > > > --- a/libavformat/asfdec_f.c > > > > +++ b/libavformat/asfdec_f.c > > > > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData *pd) > > > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content > > Description Object" > > > > * but 16 bit for "Metadata Object" and "Metadata Library > > Object" */ > > > > -static int get_value(AVIOContext *pb, int type, int type2_size) > > > > +static uint64_t get_value(AVIOContext *pb, int type, int > > type2_size) > > > > { > > > > switch (type) { > > > > case 2: > > > > @@ -568,9 +568,9 @@ static int > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > > > * ASF stream count starts at 1. I am using 0 to the > > container value > > > > * since it's unused. */ > > > > if (!strcmp(name, "AspectRatioX")) > > > > - asf->dar[0].num = get_value(s->pb, value_type, 32); > > > > + asf->dar[0].num = (int)get_value(s->pb, value_type, > > 32); > > > > else if (!strcmp(name, "AspectRatioY")) > > > > - asf->dar[0].den = get_value(s->pb, value_type, 32); > > > > + asf->dar[0].den = (int)get_value(s->pb, value_type, > > 32); > > > > else > > > > get_tag(s, name, value_type, value_len, 32); > > > > } > > > > @@ -630,11 +630,11 @@ static int > > asf_read_metadata(AVFormatContext *s, int64_t size) > > > > i, stream_num, name_len_utf16, value_type, > > value_len, name); > > > > > > > > if (!strcmp(name, "AspectRatioX")){ > > > > - int aspect_x = get_value(s->pb, value_type, 16); > > > > + int aspect_x = (int)get_value(s->pb, value_type, > > 16); > > > > if(stream_num < 128) > > > > asf->dar[stream_num].num = aspect_x; > > > > } else if(!strcmp(name, "AspectRatioY")){ > > > > - int aspect_y = get_value(s->pb, value_type, 16); > > > > + int aspect_y = (int)get_value(s->pb, value_type, > > 16); > > > > if(stream_num < 128) > > > > asf->dar[stream_num].den = aspect_y; > > > > } else { > > > > > > a 64bit/64bit aspect does not work after this, it still just > > silently > > > truncates the value to 32bit, the truncation just happens later > > > > forgot: see av_reduce() > > These lines are not about making any change. The get_value() calls > for AspectRatioX/Y are returning 16bit values, before and after. > > I added the explicit cast to (int) because get_value() doesn't return > int anymore and as such, to indicate that the cast is intentional. > But the values in this case are limited to 16bit integers anyway. I dont see what would stop a file that encodes a 64bit aspect from being read as 64bit and then randomly and silently truncated neither before nor after this patch. The type is read and passed to get_value() its not checked or i have missed it Either 64bit should be supported and converted to a supported ratio or it should produce some warning/error not silent truncation I dont understand why adding a explicit cast to int would improve anything. thx [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Michael Niedermayer > Sent: Friday, 1 October 2021 19:56 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix > get_value return type > > On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote: > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Michael Niedermayer > > > Sent: Thursday, 30 September 2021 14:04 > > > To: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: > Fix > > > get_value return type > > > > > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer > wrote: > > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > > > > get_value had a return type of int, which means that reading > > > > > QWORDS (case 4) was broken due to truncation of the result > from > > > > > avio_rl64(). > > > > > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > > > > --- > > > > > v5: Split into pieces as requested > > > > > > > > > > libavformat/asfdec_f.c | 10 +++++----- > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > > > index 72ba8b32a0..076b5ab147 100644 > > > > > --- a/libavformat/asfdec_f.c > > > > > +++ b/libavformat/asfdec_f.c > > > > > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData > *pd) > > > > > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content > > > Description Object" > > > > > * but 16 bit for "Metadata Object" and "Metadata Library > > > Object" */ > > > > > -static int get_value(AVIOContext *pb, int type, int > type2_size) > > > > > +static uint64_t get_value(AVIOContext *pb, int type, int > > > type2_size) > > > > > { > > > > > switch (type) { > > > > > case 2: > > > > > @@ -568,9 +568,9 @@ static int > > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > > > > * ASF stream count starts at 1. I am using 0 to the > > > container value > > > > > * since it's unused. */ > > > > > if (!strcmp(name, "AspectRatioX")) > > > > > - asf->dar[0].num = get_value(s->pb, value_type, > 32); > > > > > + asf->dar[0].num = (int)get_value(s->pb, > value_type, > > > 32); > > > > > else if (!strcmp(name, "AspectRatioY")) > > > > > - asf->dar[0].den = get_value(s->pb, value_type, > 32); > > > > > + asf->dar[0].den = (int)get_value(s->pb, > value_type, > > > 32); > > > > > else > > > > > get_tag(s, name, value_type, value_len, 32); > > > > > } > > > > > @@ -630,11 +630,11 @@ static int > > > asf_read_metadata(AVFormatContext *s, int64_t size) > > > > > i, stream_num, name_len_utf16, value_type, > > > value_len, name); > > > > > > > > > > if (!strcmp(name, "AspectRatioX")){ > > > > > - int aspect_x = get_value(s->pb, value_type, 16); > > > > > + int aspect_x = (int)get_value(s->pb, value_type, > > > 16); > > > > > if(stream_num < 128) > > > > > asf->dar[stream_num].num = aspect_x; > > > > > } else if(!strcmp(name, "AspectRatioY")){ > > > > > - int aspect_y = get_value(s->pb, value_type, 16); > > > > > + int aspect_y = (int)get_value(s->pb, value_type, > > > 16); > > > > > if(stream_num < 128) > > > > > asf->dar[stream_num].den = aspect_y; > > > > > } else { > > > > > > > > a 64bit/64bit aspect does not work after this, it still just > > > silently > > > > truncates the value to 32bit, the truncation just happens later > > > > > > forgot: see av_reduce() > > > > These lines are not about making any change. The get_value() calls > > for AspectRatioX/Y are returning 16bit values, before and after. > > > > I added the explicit cast to (int) because get_value() doesn't > return > > int anymore and as such, to indicate that the cast is intentional. > > But the values in this case are limited to 16bit integers anyway. > > I dont see what would stop a file that encodes a 64bit aspect from > being read as 64bit and then randomly and silently truncated > neither before nor after this patch. The type is read and passed > to get_value() its not checked or i have missed it This is correct. AspectX/Y > INT32_MAX isn't handled correctly, neither before nor after this patch. > I dont understand why adding a explicit cast to int would improve > anything. Before this patch, the return type of get_value() was int, now it's uint64_t. This causes a truncation of the return value that didn't happen before (the truncation happened in get_value()). This leads to a compiler/linter warning that hasn't existed before. Adding an explicit cast avoids that and indicates that the developer was aware of the truncation. The less warnings a code file generates, the better can a developer focus on those that might be actually relevant. That's why I've added the explicit cast. > Either 64bit should be supported and converted to a supported We can't do this because AVRational's .num and .den are of type int. > ratio or it should produce some warning/error not silent truncation The width and height fields in the ASF Specification are coded as 32bit (see Advanced Systems Format Specification, Revision 1.20.03, pages 82 and 85), so it would require a really weird encoder that emits non-reduced AspectX/Y ration component values as QWORDS. If we take such unlikely cases as a general measure, there would be quite a number of additional checks that we would need to add. This patchset deals with incorrect assumptions about realistic cases, I can add checks for the less realistic ones as well, if you wish. Thanks for looking into it, softworkz
On Fri, Oct 01, 2021 at 08:44:04PM +0000, Soft Works wrote: > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Michael Niedermayer > > Sent: Friday, 1 October 2021 19:56 > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix > > get_value return type > > > > On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote: > > > > > > > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > Michael Niedermayer > > > > Sent: Thursday, 30 September 2021 14:04 > > > > To: FFmpeg development discussions and patches <ffmpeg- > > > > devel@ffmpeg.org> > > > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: > > Fix > > > > get_value return type > > > > > > > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer > > wrote: > > > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > > > > > get_value had a return type of int, which means that reading > > > > > > QWORDS (case 4) was broken due to truncation of the result > > from > > > > > > avio_rl64(). > > > > > > > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > > > > > --- > > > > > > v5: Split into pieces as requested > > > > > > > > > > > > libavformat/asfdec_f.c | 10 +++++----- > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > > > > index 72ba8b32a0..076b5ab147 100644 > > > > > > --- a/libavformat/asfdec_f.c > > > > > > +++ b/libavformat/asfdec_f.c > > > > > > @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData > > *pd) > > > > > > > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content > > > > Description Object" > > > > > > * but 16 bit for "Metadata Object" and "Metadata Library > > > > Object" */ > > > > > > -static int get_value(AVIOContext *pb, int type, int > > type2_size) > > > > > > +static uint64_t get_value(AVIOContext *pb, int type, int > > > > type2_size) > > > > > > { > > > > > > switch (type) { > > > > > > case 2: > > > > > > @@ -568,9 +568,9 @@ static int > > > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > > > > > * ASF stream count starts at 1. I am using 0 to the > > > > container value > > > > > > * since it's unused. */ > > > > > > if (!strcmp(name, "AspectRatioX")) > > > > > > - asf->dar[0].num = get_value(s->pb, value_type, > > 32); > > > > > > + asf->dar[0].num = (int)get_value(s->pb, > > value_type, > > > > 32); > > > > > > else if (!strcmp(name, "AspectRatioY")) > > > > > > - asf->dar[0].den = get_value(s->pb, value_type, > > 32); > > > > > > + asf->dar[0].den = (int)get_value(s->pb, > > value_type, > > > > 32); > > > > > > else > > > > > > get_tag(s, name, value_type, value_len, 32); > > > > > > } > > > > > > @@ -630,11 +630,11 @@ static int > > > > asf_read_metadata(AVFormatContext *s, int64_t size) > > > > > > i, stream_num, name_len_utf16, value_type, > > > > value_len, name); > > > > > > > > > > > > if (!strcmp(name, "AspectRatioX")){ > > > > > > - int aspect_x = get_value(s->pb, value_type, 16); > > > > > > + int aspect_x = (int)get_value(s->pb, value_type, > > > > 16); > > > > > > if(stream_num < 128) > > > > > > asf->dar[stream_num].num = aspect_x; > > > > > > } else if(!strcmp(name, "AspectRatioY")){ > > > > > > - int aspect_y = get_value(s->pb, value_type, 16); > > > > > > + int aspect_y = (int)get_value(s->pb, value_type, > > > > 16); > > > > > > if(stream_num < 128) > > > > > > asf->dar[stream_num].den = aspect_y; > > > > > > } else { > > > > > > > > > > a 64bit/64bit aspect does not work after this, it still just > > > > silently > > > > > truncates the value to 32bit, the truncation just happens later > > > > > > > > forgot: see av_reduce() > > > > > > These lines are not about making any change. The get_value() calls > > > for AspectRatioX/Y are returning 16bit values, before and after. > > > > > > I added the explicit cast to (int) because get_value() doesn't > > return > > > int anymore and as such, to indicate that the cast is intentional. > > > But the values in this case are limited to 16bit integers anyway. > > > > I dont see what would stop a file that encodes a 64bit aspect from > > being read as 64bit and then randomly and silently truncated > > neither before nor after this patch. The type is read and passed > > to get_value() its not checked or i have missed it > > This is correct. AspectX/Y > INT32_MAX isn't handled correctly, > neither before nor after this patch. > > > I dont understand why adding a explicit cast to int would improve > > anything. > > Before this patch, the return type of get_value() was int, now it's > uint64_t. This causes a truncation of the return value that didn't > happen before (the truncation happened in get_value()). > > This leads to a compiler/linter warning that hasn't existed before. > Adding an explicit cast avoids that and indicates that the developer > was aware of the truncation. The less warnings a code file generates, > the better can a developer focus on those that might be actually > relevant. That's why I've added the explicit cast. whats the point of a warning ? is it to point to a potential bug ? is it a bug in this case ? if it is a bug, the warning is correct so why would any action that removes the warning without fixing the bug be good ? > > > Either 64bit should be supported and converted to a supported > > We can't do this because AVRational's .num and .den are of type int. you can certainly convert it to 32/32bit and id like to see the person that can see the difference between a 64/64bit aspect and the closest 32bit fraction to that. In fact id like to have that display that can show the difference > > > ratio or it should produce some warning/error not silent truncation > > The width and height fields in the ASF Specification are coded as > 32bit (see Advanced Systems Format Specification, Revision 1.20.03, > pages 82 and 85), so it would require a really weird encoder > that emits non-reduced AspectX/Y ration component values as QWORDS. > > If we take such unlikely cases as a general measure, there would > be quite a number of additional checks that we would need to add. > This patchset deals with incorrect assumptions about realistic cases, > I can add checks for the less realistic ones as well, if you wish. I wish that the demuxer either (approximetaly) supports the values it reads or produces warnings/errors. Not silenty misinterprets them totally One reason is, i prefer to have a warning pointing to weirdness of a file instead of having to search for where some wierdness comes from thx [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Michael Niedermayer > Sent: Saturday, 2 October 2021 13:46 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix > get_value return type > > On Fri, Oct 01, 2021 at 08:44:04PM +0000, Soft Works wrote: > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Michael Niedermayer > > > Sent: Friday, 1 October 2021 19:56 > > > To: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: > Fix > > > get_value return type > > > > > > On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On > Behalf Of > > > > > Michael Niedermayer > > > > > Sent: Thursday, 30 September 2021 14:04 > > > > > To: FFmpeg development discussions and patches <ffmpeg- > > > > > devel@ffmpeg.org> > > > > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] > libavformat/asfdec: > > > Fix > > > > > get_value return type > > > > > > > > > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer > > > wrote: > > > > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote: > > > > > > > get_value had a return type of int, which means that > reading > > > > > > > QWORDS (case 4) was broken due to truncation of the > result > > > from > > > > > > > avio_rl64(). > > > > > > > > > > > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > > > > > > --- > > > > > > > v5: Split into pieces as requested > > > > > > > > > > > > > > libavformat/asfdec_f.c | 10 +++++----- > > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/libavformat/asfdec_f.c > b/libavformat/asfdec_f.c > > > > > > > index 72ba8b32a0..076b5ab147 100644 > > > > > > > --- a/libavformat/asfdec_f.c > > > > > > > +++ b/libavformat/asfdec_f.c > > > > > > > @@ -202,7 +202,7 @@ static int asf_probe(const > AVProbeData > > > *pd) > > > > > > > > > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content > > > > > Description Object" > > > > > > > * but 16 bit for "Metadata Object" and "Metadata > Library > > > > > Object" */ > > > > > > > -static int get_value(AVIOContext *pb, int type, int > > > type2_size) > > > > > > > +static uint64_t get_value(AVIOContext *pb, int type, int > > > > > type2_size) > > > > > > > { > > > > > > > switch (type) { > > > > > > > case 2: > > > > > > > @@ -568,9 +568,9 @@ static int > > > > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size) > > > > > > > * ASF stream count starts at 1. I am using 0 to > the > > > > > container value > > > > > > > * since it's unused. */ > > > > > > > if (!strcmp(name, "AspectRatioX")) > > > > > > > - asf->dar[0].num = get_value(s->pb, > value_type, > > > 32); > > > > > > > + asf->dar[0].num = (int)get_value(s->pb, > > > value_type, > > > > > 32); > > > > > > > else if (!strcmp(name, "AspectRatioY")) > > > > > > > - asf->dar[0].den = get_value(s->pb, > value_type, > > > 32); > > > > > > > + asf->dar[0].den = (int)get_value(s->pb, > > > value_type, > > > > > 32); > > > > > > > else > > > > > > > get_tag(s, name, value_type, value_len, 32); > > > > > > > } > > > > > > > @@ -630,11 +630,11 @@ static int > > > > > asf_read_metadata(AVFormatContext *s, int64_t size) > > > > > > > i, stream_num, name_len_utf16, > value_type, > > > > > value_len, name); > > > > > > > > > > > > > > if (!strcmp(name, "AspectRatioX")){ > > > > > > > - int aspect_x = get_value(s->pb, value_type, > 16); > > > > > > > + int aspect_x = (int)get_value(s->pb, > value_type, > > > > > 16); > > > > > > > if(stream_num < 128) > > > > > > > asf->dar[stream_num].num = aspect_x; > > > > > > > } else if(!strcmp(name, "AspectRatioY")){ > > > > > > > - int aspect_y = get_value(s->pb, value_type, > 16); > > > > > > > + int aspect_y = (int)get_value(s->pb, > value_type, > > > > > 16); > > > > > > > if(stream_num < 128) > > > > > > > asf->dar[stream_num].den = aspect_y; > > > > > > > } else { > > > > > > > > > > > > a 64bit/64bit aspect does not work after this, it still > just > > > > > silently > > > > > > truncates the value to 32bit, the truncation just happens > later > > > > > > > > > > forgot: see av_reduce() > > > > > > > > These lines are not about making any change. The get_value() > calls > > > > for AspectRatioX/Y are returning 16bit values, before and > after. > > > > > > > > I added the explicit cast to (int) because get_value() doesn't > > > return > > > > int anymore and as such, to indicate that the cast is > intentional. > > > > But the values in this case are limited to 16bit integers > anyway. > > > > > > I dont see what would stop a file that encodes a 64bit aspect > from > > > being read as 64bit and then randomly and silently truncated > > > neither before nor after this patch. The type is read and passed > > > to get_value() its not checked or i have missed it > > > > This is correct. AspectX/Y > INT32_MAX isn't handled correctly, > > neither before nor after this patch. > > > > > I dont understand why adding a explicit cast to int would improve > > > anything. > > > > Before this patch, the return type of get_value() was int, now it's > > uint64_t. This causes a truncation of the return value that didn't > > happen before (the truncation happened in get_value()). > > > > > This leads to a compiler/linter warning that hasn't existed before. > > Adding an explicit cast avoids that and indicates that the > developer > > was aware of the truncation. The less warnings a code file > generates, > > the better can a developer focus on those that might be actually > > relevant. That's why I've added the explicit cast. > > whats the point of a warning ? > is it to point to a potential bug ? > is it a bug in this case ? > if it is a bug, the warning is correct > so why would any action that removes the warning without fixing the > bug be good ? It marks this as checked and considered to be unlikely to ever occur in the real world, and attention should be paid to the other warnings. My experience from many projects is that warnings only receive attention when they are being curated carefully and the overall count is low. I acknowledge though, that different philosophies exist for dealing with these things > > > Either 64bit should be supported and converted to a supported > > > > We can't do this because AVRational's .num and .den are of type > int. > > you can certainly convert it to 32/32bit and id like to see the > person > that can see the difference between a 64/64bit aspect and the closest > 32bit fraction to that. In fact id like to have that display that can > show the difference How could the DAR for an image having a 32bit w/h, require fraction components > 32bit, unless there's some little troll at work in the muxer that applies an arbitrary factor to num+den?` It's not impossible though - yes that's true. > I wish that the demuxer either (approximetaly) supports the values it > reads > or produces warnings/errors. Not silenty misinterprets them totally > One reason is, i prefer to have a warning pointing to weirdness of a > file instead of having to search for where some wierdness comes from I 100% agree to that in general! My point was that there are many other issues in this code that are much more likely to be hit than this one, but it's not worth to waste more time on that little bit. I'll add the warnings and we can put a checkmark behind it :-) Would you want me to make any other changes or should I just fire a new version? Thanks again, softworkz
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c index 72ba8b32a0..076b5ab147 100644 --- a/libavformat/asfdec_f.c +++ b/libavformat/asfdec_f.c @@ -202,7 +202,7 @@ static int asf_probe(const AVProbeData *pd) /* size of type 2 (BOOL) is 32bit for "Extended Content Description Object" * but 16 bit for "Metadata Object" and "Metadata Library Object" */ -static int get_value(AVIOContext *pb, int type, int type2_size) +static uint64_t get_value(AVIOContext *pb, int type, int type2_size) { switch (type) { case 2: @@ -568,9 +568,9 @@ static int asf_read_ext_content_desc(AVFormatContext *s, int64_t size) * ASF stream count starts at 1. I am using 0 to the container value * since it's unused. */ if (!strcmp(name, "AspectRatioX")) - asf->dar[0].num = get_value(s->pb, value_type, 32); + asf->dar[0].num = (int)get_value(s->pb, value_type, 32); else if (!strcmp(name, "AspectRatioY")) - asf->dar[0].den = get_value(s->pb, value_type, 32); + asf->dar[0].den = (int)get_value(s->pb, value_type, 32); else get_tag(s, name, value_type, value_len, 32); } @@ -630,11 +630,11 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size) i, stream_num, name_len_utf16, value_type, value_len, name); if (!strcmp(name, "AspectRatioX")){ - int aspect_x = get_value(s->pb, value_type, 16); + int aspect_x = (int)get_value(s->pb, value_type, 16); if(stream_num < 128) asf->dar[stream_num].num = aspect_x; } else if(!strcmp(name, "AspectRatioY")){ - int aspect_y = get_value(s->pb, value_type, 16); + int aspect_y = (int)get_value(s->pb, value_type, 16); if(stream_num < 128) asf->dar[stream_num].den = aspect_y; } else {
get_value had a return type of int, which means that reading QWORDS (case 4) was broken due to truncation of the result from avio_rl64(). Signed-off-by: softworkz <softworkz@hotmail.com> --- v5: Split into pieces as requested libavformat/asfdec_f.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)