[FFmpeg-devel,3/4] avformat/rmdec: Use 64bit for intermediate for DEINT_ID_INT4

Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot be represented in type 'int'
Fixes: 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/rmdec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot be represented in type 'int'
> Fixes: 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/rmdec.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index fc3bff4859..af032ed90a 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -269,9 +269,9 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>           case DEINT_ID_INT4:
>               if (ast->coded_framesize > ast->audio_framesize ||
>                   sub_packet_h <= 1 ||
> -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)

This check seems superfluous with the one below right after it. 
ast->coded_framesize * sub_packet_h must be equal to 2 * 
ast->audio_framesize. It can be removed.

>                   return AVERROR_INVALIDDATA;
> -            if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
> +            if (ast->coded_framesize * (uint64_t)sub_packet_h != 2*ast->audio_framesize) {
>                   avpriv_request_sample(s, "mismatching interleaver parameters");
>                   return AVERROR_INVALIDDATA;
>               }

How about something like

> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index fc3bff4859..09880ee3fe 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -269,7 +269,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>          case DEINT_ID_INT4:
>              if (ast->coded_framesize > ast->audio_framesize ||
>                  sub_packet_h <= 1 ||
> -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>                  return AVERROR_INVALIDDATA;
>              if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
>                  avpriv_request_sample(s, "mismatching interleaver parameters");

Instead?

We already know that ast->coded_framesize is not bigger than 
ast->audio_framesize, and with this change we'll also know that 
ast->audio_framesize * sub_packet_h can't overflow, so neither will 
ast->coded_framesize * sub_packet_h.

Michael Niedermayer:
> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot be represented in type 'int'
> Fixes: 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/rmdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index fc3bff4859..af032ed90a 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -269,9 +269,9 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>          case DEINT_ID_INT4:
>              if (ast->coded_framesize > ast->audio_framesize ||
>                  sub_packet_h <= 1 ||
> -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
>                  return AVERROR_INVALIDDATA;
> -            if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
> +            if (ast->coded_framesize * (uint64_t)sub_packet_h != 2*ast->audio_framesize) {
>                  avpriv_request_sample(s, "mismatching interleaver parameters");
>                  return AVERROR_INVALIDDATA;
>              }
> 
When I looked at Real-in-Matroska, I found the checks to be not strict
enough and one of the commits that fixed this is
bdaa98dd4aac08b8f23f959cbb5a80db2dacd14a. It disallowed sub_packet_h
being odd because otherwise one could output uninitialized data. After
all, when using INT4 deiniterleavement this demuxer reads h/2 (rounded
down) blocks of size coded_framesize each in each ff_rm_parse_packet();
and after it has done this sub_packet_h times, it outputs sub_packet_h *
audio_framesize / block_align packets of size block_align each. For
RA28.8 using INT4 deinterleavement block_align == coded_framesize. So
RA28.8 using INT4 with sub_packet_h == 3, coded_framesize == 2 and
audio_framesize == 3 will pass all checks in rmdec.c as well as all
proposed checks, yet it will allocate a packet of size 3 * 3, fill 3/2 *
2 * 3 bytes of it and output 3 * 3 / 2 = 4 packets of size 2. It is
obvious that the culprit for this is h/2 being rounded down.

But there is something here which makes this more complicated than
Matroska: The Matroska demuxer simply presumes that RA288 uses INT4
deinterleavement*; yet this seems to be not guaranteed here (there is
just a comment that INT4 is the interleavement for 28.8). Forbidding odd
sub_packet_h will (together with the current checks) make sure that we
are reading sub_packet_h/2 * sub_packet_h * coded_framesize =
sub_packet_h/2 * 2 * audio_framesize = sub_packet_h * audio_framesize,
i.e. the whole packet allocated will be initialized. But if block_align
does not divide sub_packet_h * audio_framesize, then a part of the data
read will be ignored. I don't know if this can legitimately happen; it
can't happen for RA28.8.

- Andreas

*: I don't even know whether this is required by the specs (which I have
never ever seen).

On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
> > Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot be represented in type 'int'
> > Fixes: 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/rmdec.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > index fc3bff4859..af032ed90a 100644
> > --- a/libavformat/rmdec.c
> > +++ b/libavformat/rmdec.c
> > @@ -269,9 +269,9 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> >           case DEINT_ID_INT4:
> >               if (ast->coded_framesize > ast->audio_framesize ||
> >                   sub_packet_h <= 1 ||
> > -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> > +                ast->coded_framesize * (uint64_t)sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> 
> This check seems superfluous with the one below right after it.
> ast->coded_framesize * sub_packet_h must be equal to 2 *
> ast->audio_framesize. It can be removed.
> 
> >                   return AVERROR_INVALIDDATA;
> > -            if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
> > +            if (ast->coded_framesize * (uint64_t)sub_packet_h != 2*ast->audio_framesize) {
> >                   avpriv_request_sample(s, "mismatching interleaver parameters");
> >                   return AVERROR_INVALIDDATA;
> >               }
> 
> How about something like
> 
> > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > index fc3bff4859..09880ee3fe 100644
> > --- a/libavformat/rmdec.c
> > +++ b/libavformat/rmdec.c
> > @@ -269,7 +269,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> >          case DEINT_ID_INT4:
> >              if (ast->coded_framesize > ast->audio_framesize ||
> >                  sub_packet_h <= 1 ||
> > -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
> > +                ast->audio_framesize > INT_MAX / sub_packet_h)
> >                  return AVERROR_INVALIDDATA;
> >              if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
> >                  avpriv_request_sample(s, "mismatching interleaver parameters");
> 
> Instead?

The 2 if() execute different things, the 2nd requests a sample, the first
not. I think this suggestion would change when we request a sample

thx

[...]

On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot be represented in type 'int'
>>> Fixes: 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavformat/rmdec.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>> index fc3bff4859..af032ed90a 100644
>>> --- a/libavformat/rmdec.c
>>> +++ b/libavformat/rmdec.c
>>> @@ -269,9 +269,9 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>            case DEINT_ID_INT4:
>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>                    sub_packet_h <= 1 ||
>>> -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
>>
>> This check seems superfluous with the one below right after it.
>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>> ast->audio_framesize. It can be removed.
>>
>>>                    return AVERROR_INVALIDDATA;
>>> -            if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h != 2*ast->audio_framesize) {
>>>                    avpriv_request_sample(s, "mismatching interleaver parameters");
>>>                    return AVERROR_INVALIDDATA;
>>>                }
>>
>> How about something like
>>
>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>> index fc3bff4859..09880ee3fe 100644
>>> --- a/libavformat/rmdec.c
>>> +++ b/libavformat/rmdec.c
>>> @@ -269,7 +269,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>           case DEINT_ID_INT4:
>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>                   sub_packet_h <= 1 ||
>>> -                ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
>>>                   avpriv_request_sample(s, "mismatching interleaver parameters");
>>
>> Instead?
> 
> The 2 if() execute different things, the 2nd requests a sample, the first
> not. I think this suggestion would change when we request a sample

Why are we returning INVALIDDATA after requesting a sample, for that 
matter? If it's considered an invalid scenario, do we really need a sample?

In any case, if you don't want more files where "ast->coded_framesize * 
sub_packet_h != 2*ast->audio_framesize" would print a sample request, 
then maybe something like the following could be used instead?

> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index fc3bff4859..10c1699a81 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -269,6 +269,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>          case DEINT_ID_INT4:
>              if (ast->coded_framesize > ast->audio_framesize ||
>                  sub_packet_h <= 1 ||
> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>                  ast->coded_framesize * sub_packet_h > (2 + (sub_packet_h & 1)) * ast->audio_framesize)
>                  return AVERROR_INVALIDDATA;
>              if (ast->coded_framesize * sub_packet_h != 2*ast->audio_framesize) {
> @@ -278,12 +279,16 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>              break;
>          case DEINT_ID_GENR:
>              if (ast->sub_packet_size <= 0 ||
> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>                  ast->sub_packet_size > ast->audio_framesize)
>                  return AVERROR_INVALIDDATA;
>              if (ast->audio_framesize % ast->sub_packet_size)
>                  return AVERROR_INVALIDDATA;
>              break;
>          case DEINT_ID_SIPR:
> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
> +                return AVERROR_INVALIDDATA;
> +            break;
>          case DEINT_ID_INT0:
>          case DEINT_ID_VBRS:
>          case DEINT_ID_VBRF:
> @@ -296,7 +301,6 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>              ast->deint_id == DEINT_ID_GENR ||
>              ast->deint_id == DEINT_ID_SIPR) {
>              if (st->codecpar->block_align <= 0 ||
> -                ast->audio_framesize * (uint64_t)sub_packet_h > (unsigned)INT_MAX ||
>                  ast->audio_framesize * sub_packet_h < st->codecpar->block_align)
>                  return AVERROR_INVALIDDATA;
>              if (av_new_packet(&ast->pkt, ast->audio_framesize * sub_packet_h) < 0)

Same amount of checks for all three deint ids, and no integer casting to 
prevent overflows.

James Almer:
> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>> be represented in type 'int'
>>>> Fixes:
>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>
>>>>
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>    libavformat/rmdec.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>> index fc3bff4859..af032ed90a 100644
>>>> --- a/libavformat/rmdec.c
>>>> +++ b/libavformat/rmdec.c
>>>> @@ -269,9 +269,9 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>            case DEINT_ID_INT4:
>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>                    sub_packet_h <= 1 ||
>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>
>>> This check seems superfluous with the one below right after it.
>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>> ast->audio_framesize. It can be removed.
>>>
>>>>                    return AVERROR_INVALIDDATA;
>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>> 2*ast->audio_framesize) {
>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>> 2*ast->audio_framesize) {
>>>>                    avpriv_request_sample(s, "mismatching interleaver
>>>> parameters");
>>>>                    return AVERROR_INVALIDDATA;
>>>>                }
>>>
>>> How about something like
>>>
>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>> index fc3bff4859..09880ee3fe 100644
>>>> --- a/libavformat/rmdec.c
>>>> +++ b/libavformat/rmdec.c
>>>> @@ -269,7 +269,7 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>           case DEINT_ID_INT4:
>>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>>                   sub_packet_h <= 1 ||
>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->coded_framesize * sub_packet_h !=
>>>> 2*ast->audio_framesize) {
>>>>                   avpriv_request_sample(s, "mismatching interleaver
>>>> parameters");
>>>
>>> Instead?
>>
>> The 2 if() execute different things, the 2nd requests a sample, the first
>> not. I think this suggestion would change when we request a sample
> 
> Why are we returning INVALIDDATA after requesting a sample, for that
> matter? If it's considered an invalid scenario, do we really need a sample?
> 
> In any case, if you don't want more files where "ast->coded_framesize *
> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
> then maybe something like the following could be used instead?
> 
>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>> index fc3bff4859..10c1699a81 100644
>> --- a/libavformat/rmdec.c
>> +++ b/libavformat/rmdec.c
>> @@ -269,6 +269,7 @@ static int
>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>          case DEINT_ID_INT4:
>>              if (ast->coded_framesize > ast->audio_framesize ||
>>                  sub_packet_h <= 1 ||
>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>                  ast->coded_framesize * sub_packet_h > (2 +
>> (sub_packet_h & 1)) * ast->audio_framesize)
>>                  return AVERROR_INVALIDDATA;
>>              if (ast->coded_framesize * sub_packet_h !=
>> 2*ast->audio_framesize) {
>> @@ -278,12 +279,16 @@ static int
>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>              break;
>>          case DEINT_ID_GENR:
>>              if (ast->sub_packet_size <= 0 ||
>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>                  ast->sub_packet_size > ast->audio_framesize)
>>                  return AVERROR_INVALIDDATA;
>>              if (ast->audio_framesize % ast->sub_packet_size)
>>                  return AVERROR_INVALIDDATA;
>>              break;
>>          case DEINT_ID_SIPR:
>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)

sub_packet_h has not been checked for being != 0 here and in the
DEINT_ID_GENR codepath.

>> +                return AVERROR_INVALIDDATA;
>> +            break;
>>          case DEINT_ID_INT0:
>>          case DEINT_ID_VBRS:
>>          case DEINT_ID_VBRF:
>> @@ -296,7 +301,6 @@ static int
>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>              ast->deint_id == DEINT_ID_GENR ||
>>              ast->deint_id == DEINT_ID_SIPR) {
>>              if (st->codecpar->block_align <= 0 ||
>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>> (unsigned)INT_MAX ||
>>                  ast->audio_framesize * sub_packet_h <
>> st->codecpar->block_align)
>>                  return AVERROR_INVALIDDATA;
>>              if (av_new_packet(&ast->pkt, ast->audio_framesize *
>> sub_packet_h) < 0)
> 
> Same amount of checks for all three deint ids, and no integer casting to
> prevent overflows.

Since when is a division better than casting to 64bits to perform a
multiplication?

- Andreas

On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>>> be represented in type 'int'
>>>>> Fixes:
>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>
>>>>>
>>>>> Found-by: continuous fuzzing process
>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>     libavformat/rmdec.c | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>> index fc3bff4859..af032ed90a 100644
>>>>> --- a/libavformat/rmdec.c
>>>>> +++ b/libavformat/rmdec.c
>>>>> @@ -269,9 +269,9 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>             case DEINT_ID_INT4:
>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>                     sub_packet_h <= 1 ||
>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>
>>>> This check seems superfluous with the one below right after it.
>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>> ast->audio_framesize. It can be removed.
>>>>
>>>>>                     return AVERROR_INVALIDDATA;
>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>> 2*ast->audio_framesize) {
>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>> 2*ast->audio_framesize) {
>>>>>                     avpriv_request_sample(s, "mismatching interleaver
>>>>> parameters");
>>>>>                     return AVERROR_INVALIDDATA;
>>>>>                 }
>>>>
>>>> How about something like
>>>>
>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>> index fc3bff4859..09880ee3fe 100644
>>>>> --- a/libavformat/rmdec.c
>>>>> +++ b/libavformat/rmdec.c
>>>>> @@ -269,7 +269,7 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>            case DEINT_ID_INT4:
>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>                    sub_packet_h <= 1 ||
>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>                    return AVERROR_INVALIDDATA;
>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>> 2*ast->audio_framesize) {
>>>>>                    avpriv_request_sample(s, "mismatching interleaver
>>>>> parameters");
>>>>
>>>> Instead?
>>>
>>> The 2 if() execute different things, the 2nd requests a sample, the first
>>> not. I think this suggestion would change when we request a sample
>>
>> Why are we returning INVALIDDATA after requesting a sample, for that
>> matter? If it's considered an invalid scenario, do we really need a sample?
>>
>> In any case, if you don't want more files where "ast->coded_framesize *
>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>> then maybe something like the following could be used instead?
>>
>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>> index fc3bff4859..10c1699a81 100644
>>> --- a/libavformat/rmdec.c
>>> +++ b/libavformat/rmdec.c
>>> @@ -269,6 +269,7 @@ static int
>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>           case DEINT_ID_INT4:
>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>                   sub_packet_h <= 1 ||
>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>                   ast->coded_framesize * sub_packet_h > (2 +
>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (ast->coded_framesize * sub_packet_h !=
>>> 2*ast->audio_framesize) {
>>> @@ -278,12 +279,16 @@ static int
>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>               break;
>>>           case DEINT_ID_GENR:
>>>               if (ast->sub_packet_size <= 0 ||
>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>>                   return AVERROR_INVALIDDATA;
>>>               break;
>>>           case DEINT_ID_SIPR:
>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
> 
> sub_packet_h has not been checked for being != 0 here and in the
> DEINT_ID_GENR codepath.

Ah, good catch. This also means av_new_packet() is potentially being 
called with 0 as size for these two codepaths.

> 
>>> +                return AVERROR_INVALIDDATA;
>>> +            break;
>>>           case DEINT_ID_INT0:
>>>           case DEINT_ID_VBRS:
>>>           case DEINT_ID_VBRF:
>>> @@ -296,7 +301,6 @@ static int
>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>               ast->deint_id == DEINT_ID_GENR ||
>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>               if (st->codecpar->block_align <= 0 ||
>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>> (unsigned)INT_MAX ||
>>>                   ast->audio_framesize * sub_packet_h <
>>> st->codecpar->block_align)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>> sub_packet_h) < 0)
>>
>> Same amount of checks for all three deint ids, and no integer casting to
>> prevent overflows.
> 
> Since when is a division better than casting to 64bits to perform a
> multiplication?

This is done in plenty of places across the codebase to catch the same 
kind of overflows. Does it make any measurable difference even worth 
mentioning, especially considering this is read in the header?

All these casts make the code really ugly and harder to read. Especially 
things like (unsigned)INT_MAX. So if there are cleaner solutions, they 
should be used if possible.
Code needs to not only work, but also be maintainable.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On 4/16/2021 7:45 PM, James Almer wrote:
> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>>>> be represented in type 'int'
>>>>>> Fixes:
>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576 
>>>>>>
>>>>>>
>>>>>>
>>>>>> Found-by: continuous fuzzing process
>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>> ---
>>>>>>     libavformat/rmdec.c | 4 ++--
>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>             case DEINT_ID_INT4:
>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>                     sub_packet_h <= 1 ||
>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>
>>>>> This check seems superfluous with the one below right after it.
>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>> ast->audio_framesize. It can be removed.
>>>>>
>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>>                     avpriv_request_sample(s, "mismatching interleaver
>>>>>> parameters");
>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>                 }
>>>>>
>>>>> How about something like
>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>            case DEINT_ID_INT4:
>>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>                    sub_packet_h <= 1 ||
>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>>                    avpriv_request_sample(s, "mismatching interleaver
>>>>>> parameters");
>>>>>
>>>>> Instead?
>>>>
>>>> The 2 if() execute different things, the 2nd requests a sample, the 
>>>> first
>>>> not. I think this suggestion would change when we request a sample
>>>
>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>> matter? If it's considered an invalid scenario, do we really need a 
>>> sample?
>>>
>>> In any case, if you don't want more files where "ast->coded_framesize *
>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>> then maybe something like the following could be used instead?
>>>
>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>> index fc3bff4859..10c1699a81 100644
>>>> --- a/libavformat/rmdec.c
>>>> +++ b/libavformat/rmdec.c
>>>> @@ -269,6 +269,7 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>           case DEINT_ID_INT4:
>>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>>                   sub_packet_h <= 1 ||
>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>                   ast->coded_framesize * sub_packet_h > (2 +
>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->coded_framesize * sub_packet_h !=
>>>> 2*ast->audio_framesize) {
>>>> @@ -278,12 +279,16 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               break;
>>>>           case DEINT_ID_GENR:
>>>>               if (ast->sub_packet_size <= 0 ||
>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               break;
>>>>           case DEINT_ID_SIPR:
>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>
>> sub_packet_h has not been checked for being != 0 here and in the
>> DEINT_ID_GENR codepath.
> 
> Ah, good catch. This also means av_new_packet() is potentially being 
> called with 0 as size for these two codepaths.

My bad, the check right before the av_new_packet() call makes ensures 
it's not.

> 
>>
>>>> +                return AVERROR_INVALIDDATA;
>>>> +            break;
>>>>           case DEINT_ID_INT0:
>>>>           case DEINT_ID_VBRS:
>>>>           case DEINT_ID_VBRF:
>>>> @@ -296,7 +301,6 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               ast->deint_id == DEINT_ID_GENR ||
>>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>>               if (st->codecpar->block_align <= 0 ||
>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>> (unsigned)INT_MAX ||
>>>>                   ast->audio_framesize * sub_packet_h <
>>>> st->codecpar->block_align)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>> sub_packet_h) < 0)
>>>
>>> Same amount of checks for all three deint ids, and no integer casting to
>>> prevent overflows.
>>
>> Since when is a division better than casting to 64bits to perform a
>> multiplication?
> 
> This is done in plenty of places across the codebase to catch the same 
> kind of overflows. Does it make any measurable difference even worth 
> mentioning, especially considering this is read in the header?
> 
> All these casts make the code really ugly and harder to read. Especially 
> things like (unsigned)INT_MAX. So if there are cleaner solutions, they 
> should be used if possible.
> Code needs to not only work, but also be maintainable.
> 
>>
>> - Andreas
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>

On 4/16/2021 7:45 PM, James Almer wrote:
> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>>>> be represented in type 'int'
>>>>>> Fixes:
>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576 
>>>>>>
>>>>>>
>>>>>>
>>>>>> Found-by: continuous fuzzing process
>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>> ---
>>>>>>     libavformat/rmdec.c | 4 ++--
>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>             case DEINT_ID_INT4:
>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>                     sub_packet_h <= 1 ||
>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>
>>>>> This check seems superfluous with the one below right after it.
>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>> ast->audio_framesize. It can be removed.
>>>>>
>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>>                     avpriv_request_sample(s, "mismatching interleaver
>>>>>> parameters");
>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>                 }
>>>>>
>>>>> How about something like
>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>            case DEINT_ID_INT4:
>>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>                    sub_packet_h <= 1 ||
>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>>                    avpriv_request_sample(s, "mismatching interleaver
>>>>>> parameters");
>>>>>
>>>>> Instead?
>>>>
>>>> The 2 if() execute different things, the 2nd requests a sample, the 
>>>> first
>>>> not. I think this suggestion would change when we request a sample
>>>
>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>> matter? If it's considered an invalid scenario, do we really need a 
>>> sample?
>>>
>>> In any case, if you don't want more files where "ast->coded_framesize *
>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>> then maybe something like the following could be used instead?
>>>
>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>> index fc3bff4859..10c1699a81 100644
>>>> --- a/libavformat/rmdec.c
>>>> +++ b/libavformat/rmdec.c
>>>> @@ -269,6 +269,7 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>           case DEINT_ID_INT4:
>>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>>                   sub_packet_h <= 1 ||
>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>                   ast->coded_framesize * sub_packet_h > (2 +
>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->coded_framesize * sub_packet_h !=
>>>> 2*ast->audio_framesize) {
>>>> @@ -278,12 +279,16 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               break;
>>>>           case DEINT_ID_GENR:
>>>>               if (ast->sub_packet_size <= 0 ||
>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               break;
>>>>           case DEINT_ID_SIPR:
>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>
>> sub_packet_h has not been checked for being != 0 here and in the
>> DEINT_ID_GENR codepath.
> 
> Ah, good catch. This also means av_new_packet() is potentially being 
> called with 0 as size for these two codepaths.
> 
>>
>>>> +                return AVERROR_INVALIDDATA;
>>>> +            break;
>>>>           case DEINT_ID_INT0:
>>>>           case DEINT_ID_VBRS:
>>>>           case DEINT_ID_VBRF:
>>>> @@ -296,7 +301,6 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               ast->deint_id == DEINT_ID_GENR ||
>>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>>               if (st->codecpar->block_align <= 0 ||
>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>> (unsigned)INT_MAX ||
>>>>                   ast->audio_framesize * sub_packet_h <
>>>> st->codecpar->block_align)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>> sub_packet_h) < 0)
>>>
>>> Same amount of checks for all three deint ids, and no integer casting to
>>> prevent overflows.
>>
>> Since when is a division better than casting to 64bits to perform a
>> multiplication?
> 
> This is done in plenty of places across the codebase to catch the same 
> kind of overflows. Does it make any measurable difference even worth 
> mentioning, especially considering this is read in the header?
> 
> All these casts make the code really ugly and harder to read. Especially 
> things like (unsigned)INT_MAX. So if there are cleaner solutions, they 
> should be used if possible.
> Code needs to not only work, but also be maintainable.

Another option is to just change the type of the RMStream fields, like so:

> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index fc3bff4859..304984d2b0 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -50,8 +50,8 @@ struct RMStream {
>      /// Audio descrambling matrix parameters
>      int64_t audiotimestamp; ///< Audio packet timestamp
>      int sub_packet_cnt; // Subpacket counter, used while reading
> -    int sub_packet_size, sub_packet_h, coded_framesize; ///< Descrambling parameters from container
> -    int audio_framesize; /// Audio frame size from container
> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///< Descrambling parameters from container
> +    unsigned audio_framesize; /// Audio frame size from container
>      int sub_packet_lengths[16]; /// Length of each subpacket
>      int32_t deint_id;  ///< deinterleaver used in audio stream
>  };
> @@ -277,7 +277,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>              }
>              break;
>          case DEINT_ID_GENR:
> -            if (ast->sub_packet_size <= 0 ||
> +            if (!ast->sub_packet_size ||
>                  ast->sub_packet_size > ast->audio_framesize)
>                  return AVERROR_INVALIDDATA;
>              if (ast->audio_framesize % ast->sub_packet_size)
> @@ -296,7 +296,7 @@ static int rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>              ast->deint_id == DEINT_ID_GENR ||
>              ast->deint_id == DEINT_ID_SIPR) {
>              if (st->codecpar->block_align <= 0 ||
> -                ast->audio_framesize * (uint64_t)sub_packet_h > (unsigned)INT_MAX ||
> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>                  ast->audio_framesize * sub_packet_h < st->codecpar->block_align)
>                  return AVERROR_INVALIDDATA;
>              if (av_new_packet(&ast->pkt, ast->audio_framesize * sub_packet_h) < 0)

ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX, 
so unless I'm missing something, this should be enough.

James Almer:
> On 4/16/2021 7:45 PM, James Almer wrote:
>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>>>>> be represented in type 'int'
>>>>>>> Fixes:
>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Found-by: continuous fuzzing process
>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>> ---
>>>>>>>     libavformat/rmdec.c | 4 ++--
>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>> --- a/libavformat/rmdec.c
>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>             case DEINT_ID_INT4:
>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>                     sub_packet_h <= 1 ||
>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>
>>>>>> This check seems superfluous with the one below right after it.
>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>> ast->audio_framesize. It can be removed.
>>>>>>
>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>> 2*ast->audio_framesize) {
>>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>>> 2*ast->audio_framesize) {
>>>>>>>                     avpriv_request_sample(s, "mismatching
>>>>>>> interleaver
>>>>>>> parameters");
>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>                 }
>>>>>>
>>>>>> How about something like
>>>>>>
>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>> --- a/libavformat/rmdec.c
>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>            case DEINT_ID_INT4:
>>>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>                    sub_packet_h <= 1 ||
>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>>>> 2*ast->audio_framesize) {
>>>>>>>                    avpriv_request_sample(s, "mismatching interleaver
>>>>>>> parameters");
>>>>>>
>>>>>> Instead?
>>>>>
>>>>> The 2 if() execute different things, the 2nd requests a sample, the
>>>>> first
>>>>> not. I think this suggestion would change when we request a sample
>>>>
>>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>>> matter? If it's considered an invalid scenario, do we really need a
>>>> sample?
>>>>
>>>> In any case, if you don't want more files where "ast->coded_framesize *
>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>>> then maybe something like the following could be used instead?
>>>>
>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>> index fc3bff4859..10c1699a81 100644
>>>>> --- a/libavformat/rmdec.c
>>>>> +++ b/libavformat/rmdec.c
>>>>> @@ -269,6 +269,7 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>           case DEINT_ID_INT4:
>>>>>               if (ast->coded_framesize > ast->audio_framesize ||
>>>>>                   sub_packet_h <= 1 ||
>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>                   ast->coded_framesize * sub_packet_h > (2 +
>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>                   return AVERROR_INVALIDDATA;
>>>>>               if (ast->coded_framesize * sub_packet_h !=
>>>>> 2*ast->audio_framesize) {
>>>>> @@ -278,12 +279,16 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>               break;
>>>>>           case DEINT_ID_GENR:
>>>>>               if (ast->sub_packet_size <= 0 ||
>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>>>                   return AVERROR_INVALIDDATA;
>>>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>>>>                   return AVERROR_INVALIDDATA;
>>>>>               break;
>>>>>           case DEINT_ID_SIPR:
>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>
>>> sub_packet_h has not been checked for being != 0 here and in the
>>> DEINT_ID_GENR codepath.
>>
>> Ah, good catch. This also means av_new_packet() is potentially being
>> called with 0 as size for these two codepaths.
>>
>>>
>>>>> +                return AVERROR_INVALIDDATA;
>>>>> +            break;
>>>>>           case DEINT_ID_INT0:
>>>>>           case DEINT_ID_VBRS:
>>>>>           case DEINT_ID_VBRF:
>>>>> @@ -296,7 +301,6 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>               ast->deint_id == DEINT_ID_GENR ||
>>>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>>>               if (st->codecpar->block_align <= 0 ||
>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>> (unsigned)INT_MAX ||
>>>>>                   ast->audio_framesize * sub_packet_h <
>>>>> st->codecpar->block_align)
>>>>>                   return AVERROR_INVALIDDATA;
>>>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>> sub_packet_h) < 0)
>>>>
>>>> Same amount of checks for all three deint ids, and no integer
>>>> casting to
>>>> prevent overflows.
>>>
>>> Since when is a division better than casting to 64bits to perform a
>>> multiplication?
>>
>> This is done in plenty of places across the codebase to catch the same
>> kind of overflows. Does it make any measurable difference even worth
>> mentioning, especially considering this is read in the header?
>>
>> All these casts make the code really ugly and harder to read.
>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>> solutions, they should be used if possible.
>> Code needs to not only work, but also be maintainable.
> 
> Another option is to just change the type of the RMStream fields, like so:
> 
>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>> index fc3bff4859..304984d2b0 100644
>> --- a/libavformat/rmdec.c
>> +++ b/libavformat/rmdec.c
>> @@ -50,8 +50,8 @@ struct RMStream {
>>      /// Audio descrambling matrix parameters
>>      int64_t audiotimestamp; ///< Audio packet timestamp
>>      int sub_packet_cnt; // Subpacket counter, used while reading
>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>> Descrambling parameters from container
>> -    int audio_framesize; /// Audio frame size from container
>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>> Descrambling parameters from container
>> +    unsigned audio_framesize; /// Audio frame size from container
>>      int sub_packet_lengths[16]; /// Length of each subpacket
>>      int32_t deint_id;  ///< deinterleaver used in audio stream
>>  };
>> @@ -277,7 +277,7 @@ static int
>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>              }
>>              break;
>>          case DEINT_ID_GENR:
>> -            if (ast->sub_packet_size <= 0 ||
>> +            if (!ast->sub_packet_size ||
>>                  ast->sub_packet_size > ast->audio_framesize)
>>                  return AVERROR_INVALIDDATA;
>>              if (ast->audio_framesize % ast->sub_packet_size)
>> @@ -296,7 +296,7 @@ static int
>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>              ast->deint_id == DEINT_ID_GENR ||
>>              ast->deint_id == DEINT_ID_SIPR) {
>>              if (st->codecpar->block_align <= 0 ||
>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>> (unsigned)INT_MAX ||
>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>                  ast->audio_framesize * sub_packet_h <
>> st->codecpar->block_align)
>>                  return AVERROR_INVALIDDATA;
>>              if (av_new_packet(&ast->pkt, ast->audio_framesize *
>> sub_packet_h) < 0)
> 
> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
> so unless I'm missing something, this should be enough.

In the multiplication ast->coded_framesize * sub_packet_h the first is
read via av_rb32(). Your patch will indeed eliminate the undefined
behaviour (because unsigned), but it might be that the check will now
not trigger when it should trigger because only the lower 32bits are
compared.

- Andreas

On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/16/2021 7:45 PM, James Almer wrote:
>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
>>>>>>>> be represented in type 'int'
>>>>>>>> Fixes:
>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>> ---
>>>>>>>>      libavformat/rmdec.c | 4 ++--
>>>>>>>>      1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>                  if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>
>>>>>>> This check seems superfluous with the one below right after it.
>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>
>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>                      avpriv_request_sample(s, "mismatching
>>>>>>>> interleaver
>>>>>>>> parameters");
>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>                  }
>>>>>>>
>>>>>>> How about something like
>>>>>>>
>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>             case DEINT_ID_INT4:
>>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>                     sub_packet_h <= 1 ||
>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>                 if (ast->coded_framesize * sub_packet_h !=
>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>                     avpriv_request_sample(s, "mismatching interleaver
>>>>>>>> parameters");
>>>>>>>
>>>>>>> Instead?
>>>>>>
>>>>>> The 2 if() execute different things, the 2nd requests a sample, the
>>>>>> first
>>>>>> not. I think this suggestion would change when we request a sample
>>>>>
>>>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>>>> matter? If it's considered an invalid scenario, do we really need a
>>>>> sample?
>>>>>
>>>>> In any case, if you don't want more files where "ast->coded_framesize *
>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>>>> then maybe something like the following could be used instead?
>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>            case DEINT_ID_INT4:
>>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>                    sub_packet_h <= 1 ||
>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>                    ast->coded_framesize * sub_packet_h > (2 +
>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>>> 2*ast->audio_framesize) {
>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>                break;
>>>>>>            case DEINT_ID_GENR:
>>>>>>                if (ast->sub_packet_size <= 0 ||
>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>                    ast->sub_packet_size > ast->audio_framesize)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                break;
>>>>>>            case DEINT_ID_SIPR:
>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>
>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>> DEINT_ID_GENR codepath.
>>>
>>> Ah, good catch. This also means av_new_packet() is potentially being
>>> called with 0 as size for these two codepaths.
>>>
>>>>
>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>> +            break;
>>>>>>            case DEINT_ID_INT0:
>>>>>>            case DEINT_ID_VBRS:
>>>>>>            case DEINT_ID_VBRF:
>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>                ast->deint_id == DEINT_ID_GENR ||
>>>>>>                ast->deint_id == DEINT_ID_SIPR) {
>>>>>>                if (st->codecpar->block_align <= 0 ||
>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>> (unsigned)INT_MAX ||
>>>>>>                    ast->audio_framesize * sub_packet_h <
>>>>>> st->codecpar->block_align)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>>> sub_packet_h) < 0)
>>>>>
>>>>> Same amount of checks for all three deint ids, and no integer
>>>>> casting to
>>>>> prevent overflows.
>>>>
>>>> Since when is a division better than casting to 64bits to perform a
>>>> multiplication?
>>>
>>> This is done in plenty of places across the codebase to catch the same
>>> kind of overflows. Does it make any measurable difference even worth
>>> mentioning, especially considering this is read in the header?
>>>
>>> All these casts make the code really ugly and harder to read.
>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>> solutions, they should be used if possible.
>>> Code needs to not only work, but also be maintainable.
>>
>> Another option is to just change the type of the RMStream fields, like so:
>>
>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>> index fc3bff4859..304984d2b0 100644
>>> --- a/libavformat/rmdec.c
>>> +++ b/libavformat/rmdec.c
>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>       /// Audio descrambling matrix parameters
>>>       int64_t audiotimestamp; ///< Audio packet timestamp
>>>       int sub_packet_cnt; // Subpacket counter, used while reading
>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>> Descrambling parameters from container
>>> -    int audio_framesize; /// Audio frame size from container
>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>>> Descrambling parameters from container
>>> +    unsigned audio_framesize; /// Audio frame size from container
>>>       int sub_packet_lengths[16]; /// Length of each subpacket
>>>       int32_t deint_id;  ///< deinterleaver used in audio stream
>>>   };
>>> @@ -277,7 +277,7 @@ static int
>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>               }
>>>               break;
>>>           case DEINT_ID_GENR:
>>> -            if (ast->sub_packet_size <= 0 ||
>>> +            if (!ast->sub_packet_size ||
>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>> @@ -296,7 +296,7 @@ static int
>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>               ast->deint_id == DEINT_ID_GENR ||
>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>               if (st->codecpar->block_align <= 0 ||
>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>> (unsigned)INT_MAX ||
>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>                   ast->audio_framesize * sub_packet_h <
>>> st->codecpar->block_align)
>>>                   return AVERROR_INVALIDDATA;
>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>> sub_packet_h) < 0)
>>
>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
>> so unless I'm missing something, this should be enough.
> 
> In the multiplication ast->coded_framesize * sub_packet_h the first is
> read via av_rb32(). Your patch will indeed eliminate the undefined
> behaviour (because unsigned), but it might be that the check will now
> not trigger when it should trigger because only the lower 32bits are
> compared.

ast->coded_framesize is guaranteed to be less than or equal to 
ast->audio_framesize, which is guaranteed to be at most INT16_MAX.

> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

James Almer:
> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 4/16/2021 7:45 PM, James Almer wrote:
>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
>>>>>>>>> cannot
>>>>>>>>> be represented in type 'int'
>>>>>>>>> Fixes:
>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>> ---
>>>>>>>>>      libavformat/rmdec.c | 4 ++--
>>>>>>>>>      1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>>                  if (ast->coded_framesize >
>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h
>>>>>>>>> > (2
>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>
>>>>>>>> This check seems superfluous with the one below right after it.
>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>>
>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>                      avpriv_request_sample(s, "mismatching
>>>>>>>>> interleaver
>>>>>>>>> parameters");
>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>                  }
>>>>>>>>
>>>>>>>> How about something like
>>>>>>>>
>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>             case DEINT_ID_INT4:
>>>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>>                     sub_packet_h <= 1 ||
>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>                     avpriv_request_sample(s, "mismatching
>>>>>>>>> interleaver
>>>>>>>>> parameters");
>>>>>>>>
>>>>>>>> Instead?
>>>>>>>
>>>>>>> The 2 if() execute different things, the 2nd requests a sample, the
>>>>>>> first
>>>>>>> not. I think this suggestion would change when we request a sample
>>>>>>
>>>>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>>>>> matter? If it's considered an invalid scenario, do we really need a
>>>>>> sample?
>>>>>>
>>>>>> In any case, if you don't want more files where
>>>>>> "ast->coded_framesize *
>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>>>>> then maybe something like the following could be used instead?
>>>>>>
>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>>> --- a/libavformat/rmdec.c
>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>            case DEINT_ID_INT4:
>>>>>>>                if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>                    sub_packet_h <= 1 ||
>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>                    ast->coded_framesize * sub_packet_h > (2 +
>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>>                if (ast->coded_framesize * sub_packet_h !=
>>>>>>> 2*ast->audio_framesize) {
>>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>                break;
>>>>>>>            case DEINT_ID_GENR:
>>>>>>>                if (ast->sub_packet_size <= 0 ||
>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>                    ast->sub_packet_size > ast->audio_framesize)
>>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>>                if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>>                break;
>>>>>>>            case DEINT_ID_SIPR:
>>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>
>>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>>> DEINT_ID_GENR codepath.
>>>>
>>>> Ah, good catch. This also means av_new_packet() is potentially being
>>>> called with 0 as size for these two codepaths.
>>>>
>>>>>
>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>> +            break;
>>>>>>>            case DEINT_ID_INT0:
>>>>>>>            case DEINT_ID_VBRS:
>>>>>>>            case DEINT_ID_VBRF:
>>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>                ast->deint_id == DEINT_ID_GENR ||
>>>>>>>                ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>                if (st->codecpar->block_align <= 0 ||
>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>> (unsigned)INT_MAX ||
>>>>>>>                    ast->audio_framesize * sub_packet_h <
>>>>>>> st->codecpar->block_align)
>>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>>                if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>>>> sub_packet_h) < 0)
>>>>>>
>>>>>> Same amount of checks for all three deint ids, and no integer
>>>>>> casting to
>>>>>> prevent overflows.
>>>>>
>>>>> Since when is a division better than casting to 64bits to perform a
>>>>> multiplication?
>>>>
>>>> This is done in plenty of places across the codebase to catch the same
>>>> kind of overflows. Does it make any measurable difference even worth
>>>> mentioning, especially considering this is read in the header?
>>>>
>>>> All these casts make the code really ugly and harder to read.
>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>>> solutions, they should be used if possible.
>>>> Code needs to not only work, but also be maintainable.
>>>
>>> Another option is to just change the type of the RMStream fields,
>>> like so:
>>>
>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>> index fc3bff4859..304984d2b0 100644
>>>> --- a/libavformat/rmdec.c
>>>> +++ b/libavformat/rmdec.c
>>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>>       /// Audio descrambling matrix parameters
>>>>       int64_t audiotimestamp; ///< Audio packet timestamp
>>>>       int sub_packet_cnt; // Subpacket counter, used while reading
>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>> Descrambling parameters from container
>>>> -    int audio_framesize; /// Audio frame size from container
>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>> Descrambling parameters from container
>>>> +    unsigned audio_framesize; /// Audio frame size from container
>>>>       int sub_packet_lengths[16]; /// Length of each subpacket
>>>>       int32_t deint_id;  ///< deinterleaver used in audio stream
>>>>   };
>>>> @@ -277,7 +277,7 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               }
>>>>               break;
>>>>           case DEINT_ID_GENR:
>>>> -            if (ast->sub_packet_size <= 0 ||
>>>> +            if (!ast->sub_packet_size ||
>>>>                   ast->sub_packet_size > ast->audio_framesize)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (ast->audio_framesize % ast->sub_packet_size)
>>>> @@ -296,7 +296,7 @@ static int
>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>               ast->deint_id == DEINT_ID_GENR ||
>>>>               ast->deint_id == DEINT_ID_SIPR) {
>>>>               if (st->codecpar->block_align <= 0 ||
>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>> (unsigned)INT_MAX ||
>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>>                   ast->audio_framesize * sub_packet_h <
>>>> st->codecpar->block_align)
>>>>                   return AVERROR_INVALIDDATA;
>>>>               if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>> sub_packet_h) < 0)
>>>
>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
>>> so unless I'm missing something, this should be enough.
>>
>> In the multiplication ast->coded_framesize * sub_packet_h the first is
>> read via av_rb32(). Your patch will indeed eliminate the undefined
>> behaviour (because unsigned), but it might be that the check will now
>> not trigger when it should trigger because only the lower 32bits are
>> compared.
> 
> ast->coded_framesize is guaranteed to be less than or equal to
> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
> 

True (apart from the bound being UINT16_MAX). Doesn't fix the
uninitialized data that I mentioned though.
Yet there is a check for coded_framesize being < 0 immediately after it
is read. Said check would be moot with your changes. The problem is that
if its value is not representable as an int, one could set a negative
block_align value based upon it.

- Andreas

On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 4/16/2021 7:45 PM, James Almer wrote:
>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
>>>>>>>>>> cannot
>>>>>>>>>> be represented in type 'int'
>>>>>>>>>> Fixes:
>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>> ---
>>>>>>>>>>       libavformat/rmdec.c | 4 ++--
>>>>>>>>>>       1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>               case DEINT_ID_INT4:
>>>>>>>>>>                   if (ast->coded_framesize >
>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>                       sub_packet_h <= 1 ||
>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h
>>>>>>>>>>> (2
>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>
>>>>>>>>> This check seems superfluous with the one below right after it.
>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>>>
>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>> +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>                       avpriv_request_sample(s, "mismatching
>>>>>>>>>> interleaver
>>>>>>>>>> parameters");
>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>>                   }
>>>>>>>>>
>>>>>>>>> How about something like
>>>>>>>>>
>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>>>                  if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>                  if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>                      avpriv_request_sample(s, "mismatching
>>>>>>>>>> interleaver
>>>>>>>>>> parameters");
>>>>>>>>>
>>>>>>>>> Instead?
>>>>>>>>
>>>>>>>> The 2 if() execute different things, the 2nd requests a sample, the
>>>>>>>> first
>>>>>>>> not. I think this suggestion would change when we request a sample
>>>>>>>
>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for that
>>>>>>> matter? If it's considered an invalid scenario, do we really need a
>>>>>>> sample?
>>>>>>>
>>>>>>> In any case, if you don't want more files where
>>>>>>> "ast->coded_framesize *
>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample request,
>>>>>>> then maybe something like the following could be used instead?
>>>>>>>
>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>             case DEINT_ID_INT4:
>>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>                     sub_packet_h <= 1 ||
>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>                     ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>                 if (ast->coded_framesize * sub_packet_h !=
>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>                 break;
>>>>>>>>             case DEINT_ID_GENR:
>>>>>>>>                 if (ast->sub_packet_size <= 0 ||
>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>                     ast->sub_packet_size > ast->audio_framesize)
>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>                 if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>                 break;
>>>>>>>>             case DEINT_ID_SIPR:
>>>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>
>>>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>>>> DEINT_ID_GENR codepath.
>>>>>
>>>>> Ah, good catch. This also means av_new_packet() is potentially being
>>>>> called with 0 as size for these two codepaths.
>>>>>
>>>>>>
>>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>>> +            break;
>>>>>>>>             case DEINT_ID_INT0:
>>>>>>>>             case DEINT_ID_VBRS:
>>>>>>>>             case DEINT_ID_VBRF:
>>>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>                 ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>                 ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>                 if (st->codecpar->block_align <= 0 ||
>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>                     ast->audio_framesize * sub_packet_h <
>>>>>>>> st->codecpar->block_align)
>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>                 if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>>>>> sub_packet_h) < 0)
>>>>>>>
>>>>>>> Same amount of checks for all three deint ids, and no integer
>>>>>>> casting to
>>>>>>> prevent overflows.
>>>>>>
>>>>>> Since when is a division better than casting to 64bits to perform a
>>>>>> multiplication?
>>>>>
>>>>> This is done in plenty of places across the codebase to catch the same
>>>>> kind of overflows. Does it make any measurable difference even worth
>>>>> mentioning, especially considering this is read in the header?
>>>>>
>>>>> All these casts make the code really ugly and harder to read.
>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>>>> solutions, they should be used if possible.
>>>>> Code needs to not only work, but also be maintainable.
>>>>
>>>> Another option is to just change the type of the RMStream fields,
>>>> like so:
>>>>
>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>> index fc3bff4859..304984d2b0 100644
>>>>> --- a/libavformat/rmdec.c
>>>>> +++ b/libavformat/rmdec.c
>>>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>>>        /// Audio descrambling matrix parameters
>>>>>        int64_t audiotimestamp; ///< Audio packet timestamp
>>>>>        int sub_packet_cnt; // Subpacket counter, used while reading
>>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>> Descrambling parameters from container
>>>>> -    int audio_framesize; /// Audio frame size from container
>>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>> Descrambling parameters from container
>>>>> +    unsigned audio_framesize; /// Audio frame size from container
>>>>>        int sub_packet_lengths[16]; /// Length of each subpacket
>>>>>        int32_t deint_id;  ///< deinterleaver used in audio stream
>>>>>    };
>>>>> @@ -277,7 +277,7 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>                }
>>>>>                break;
>>>>>            case DEINT_ID_GENR:
>>>>> -            if (ast->sub_packet_size <= 0 ||
>>>>> +            if (!ast->sub_packet_size ||
>>>>>                    ast->sub_packet_size > ast->audio_framesize)
>>>>>                    return AVERROR_INVALIDDATA;
>>>>>                if (ast->audio_framesize % ast->sub_packet_size)
>>>>> @@ -296,7 +296,7 @@ static int
>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>                ast->deint_id == DEINT_ID_GENR ||
>>>>>                ast->deint_id == DEINT_ID_SIPR) {
>>>>>                if (st->codecpar->block_align <= 0 ||
>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>> (unsigned)INT_MAX ||
>>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>>>                    ast->audio_framesize * sub_packet_h <
>>>>> st->codecpar->block_align)
>>>>>                    return AVERROR_INVALIDDATA;
>>>>>                if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>> sub_packet_h) < 0)
>>>>
>>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
>>>> so unless I'm missing something, this should be enough.
>>>
>>> In the multiplication ast->coded_framesize * sub_packet_h the first is
>>> read via av_rb32(). Your patch will indeed eliminate the undefined
>>> behaviour (because unsigned), but it might be that the check will now
>>> not trigger when it should trigger because only the lower 32bits are
>>> compared.
>>
>> ast->coded_framesize is guaranteed to be less than or equal to
>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
>>
> 
> True (apart from the bound being UINT16_MAX).

Yes, my bad.

  Doesn't fix the
> uninitialized data that I mentioned though.
> Yet there is a check for coded_framesize being < 0 immediately after it
> is read. Said check would be moot with your changes. The problem is that
> if its value is not representable as an int, one could set a negative
> block_align value based upon it.

With coded_framesize being an int (local variable where the value is 
read with avio_rb32()) and ast->coded_framesize being unsigned (context 
variable where the value is ultimately stored), the end result after the 
< 0 check will be that ast->coded_framesize is at most INT_MAX right 
from the beginning, so block_align can't be negative either.

James Almer:
> On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 4/16/2021 7:45 PM, James Almer wrote:
>>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
>>>>>>>>>>> cannot
>>>>>>>>>>> be represented in type 'int'
>>>>>>>>>>> Fixes:
>>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>>> ---
>>>>>>>>>>>       libavformat/rmdec.c | 4 ++--
>>>>>>>>>>>       1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>               case DEINT_ID_INT4:
>>>>>>>>>>>                   if (ast->coded_framesize >
>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>                       sub_packet_h <= 1 ||
>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h
>>>>>>>>>>>> (2
>>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>
>>>>>>>>>> This check seems superfluous with the one below right after it.
>>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>>>>
>>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>> +            if (ast->coded_framesize *
>>>>>>>>>>> (uint64_t)sub_packet_h !=
>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>                       avpriv_request_sample(s, "mismatching
>>>>>>>>>>> interleaver
>>>>>>>>>>> parameters");
>>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>>>                   }
>>>>>>>>>>
>>>>>>>>>> How about something like
>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>>>>                  if (ast->coded_framesize >
>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>                  if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>                      avpriv_request_sample(s, "mismatching
>>>>>>>>>>> interleaver
>>>>>>>>>>> parameters");
>>>>>>>>>>
>>>>>>>>>> Instead?
>>>>>>>>>
>>>>>>>>> The 2 if() execute different things, the 2nd requests a sample,
>>>>>>>>> the
>>>>>>>>> first
>>>>>>>>> not. I think this suggestion would change when we request a sample
>>>>>>>>
>>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for
>>>>>>>> that
>>>>>>>> matter? If it's considered an invalid scenario, do we really need a
>>>>>>>> sample?
>>>>>>>>
>>>>>>>> In any case, if you don't want more files where
>>>>>>>> "ast->coded_framesize *
>>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample
>>>>>>>> request,
>>>>>>>> then maybe something like the following could be used instead?
>>>>>>>>
>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>             case DEINT_ID_INT4:
>>>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>>                     sub_packet_h <= 1 ||
>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>>                     ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>                 break;
>>>>>>>>>             case DEINT_ID_GENR:
>>>>>>>>>                 if (ast->sub_packet_size <= 0 ||
>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>>                     ast->sub_packet_size > ast->audio_framesize)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 break;
>>>>>>>>>             case DEINT_ID_SIPR:
>>>>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>
>>>>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>>>>> DEINT_ID_GENR codepath.
>>>>>>
>>>>>> Ah, good catch. This also means av_new_packet() is potentially being
>>>>>> called with 0 as size for these two codepaths.
>>>>>>
>>>>>>>
>>>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>>>> +            break;
>>>>>>>>>             case DEINT_ID_INT0:
>>>>>>>>>             case DEINT_ID_VBRS:
>>>>>>>>>             case DEINT_ID_VBRF:
>>>>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>                 ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>>                 ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>>                 if (st->codecpar->block_align <= 0 ||
>>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>>                     ast->audio_framesize * sub_packet_h <
>>>>>>>>> st->codecpar->block_align)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (av_new_packet(&ast->pkt,
>>>>>>>>> ast->audio_framesize *
>>>>>>>>> sub_packet_h) < 0)
>>>>>>>>
>>>>>>>> Same amount of checks for all three deint ids, and no integer
>>>>>>>> casting to
>>>>>>>> prevent overflows.
>>>>>>>
>>>>>>> Since when is a division better than casting to 64bits to perform a
>>>>>>> multiplication?
>>>>>>
>>>>>> This is done in plenty of places across the codebase to catch the
>>>>>> same
>>>>>> kind of overflows. Does it make any measurable difference even worth
>>>>>> mentioning, especially considering this is read in the header?
>>>>>>
>>>>>> All these casts make the code really ugly and harder to read.
>>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>>>>> solutions, they should be used if possible.
>>>>>> Code needs to not only work, but also be maintainable.
>>>>>
>>>>> Another option is to just change the type of the RMStream fields,
>>>>> like so:
>>>>>
>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>> index fc3bff4859..304984d2b0 100644
>>>>>> --- a/libavformat/rmdec.c
>>>>>> +++ b/libavformat/rmdec.c
>>>>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>>>>        /// Audio descrambling matrix parameters
>>>>>>        int64_t audiotimestamp; ///< Audio packet timestamp
>>>>>>        int sub_packet_cnt; // Subpacket counter, used while reading
>>>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>>> Descrambling parameters from container
>>>>>> -    int audio_framesize; /// Audio frame size from container
>>>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>>> Descrambling parameters from container
>>>>>> +    unsigned audio_framesize; /// Audio frame size from container
>>>>>>        int sub_packet_lengths[16]; /// Length of each subpacket
>>>>>>        int32_t deint_id;  ///< deinterleaver used in audio stream
>>>>>>    };
>>>>>> @@ -277,7 +277,7 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>                }
>>>>>>                break;
>>>>>>            case DEINT_ID_GENR:
>>>>>> -            if (ast->sub_packet_size <= 0 ||
>>>>>> +            if (!ast->sub_packet_size ||
>>>>>>                    ast->sub_packet_size > ast->audio_framesize)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (ast->audio_framesize % ast->sub_packet_size)
>>>>>> @@ -296,7 +296,7 @@ static int
>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>                ast->deint_id == DEINT_ID_GENR ||
>>>>>>                ast->deint_id == DEINT_ID_SIPR) {
>>>>>>                if (st->codecpar->block_align <= 0 ||
>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>> (unsigned)INT_MAX ||
>>>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>>>>                    ast->audio_framesize * sub_packet_h <
>>>>>> st->codecpar->block_align)
>>>>>>                    return AVERROR_INVALIDDATA;
>>>>>>                if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>>> sub_packet_h) < 0)
>>>>>
>>>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
>>>>> so unless I'm missing something, this should be enough.
>>>>
>>>> In the multiplication ast->coded_framesize * sub_packet_h the first is
>>>> read via av_rb32(). Your patch will indeed eliminate the undefined
>>>> behaviour (because unsigned), but it might be that the check will now
>>>> not trigger when it should trigger because only the lower 32bits are
>>>> compared.
>>>
>>> ast->coded_framesize is guaranteed to be less than or equal to
>>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
>>>
>>
>> True (apart from the bound being UINT16_MAX).
> 
> Yes, my bad.
> 
>  Doesn't fix the
>> uninitialized data that I mentioned though.
>> Yet there is a check for coded_framesize being < 0 immediately after it
>> is read. Said check would be moot with your changes. The problem is that
>> if its value is not representable as an int, one could set a negative
>> block_align value based upon it.
> 
> With coded_framesize being an int (local variable where the value is
> read with avio_rb32()) and ast->coded_framesize being unsigned (context
> variable where the value is ultimately stored), the end result after the
> < 0 check will be that ast->coded_framesize is at most INT_MAX right
> from the beginning, so block_align can't be negative either.

True, the check uses a local int variable.

- Andreas

On Fri, Apr 16, 2021 at 08:37:51PM -0300, James Almer wrote:
> On 4/16/2021 7:45 PM, James Almer wrote:
> > On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
> > > James Almer:
> > > > On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
> > > > > On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
> > > > > > On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
> > > > > > > Fixes: runtime error: signed integer overflow: 65312 * 65535 cannot
> > > > > > > be represented in type 'int'
> > > > > > > Fixes:
> > > > > > > 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Found-by: continuous fuzzing process
> > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > > ---
> > > > > > >     libavformat/rmdec.c | 4 ++--
> > > > > > >     1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > > > > > > index fc3bff4859..af032ed90a 100644
> > > > > > > --- a/libavformat/rmdec.c
> > > > > > > +++ b/libavformat/rmdec.c
> > > > > > > @@ -269,9 +269,9 @@ static int
> > > > > > > rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > > > > > >             case DEINT_ID_INT4:
> > > > > > >                 if (ast->coded_framesize > ast->audio_framesize ||
> > > > > > >                     sub_packet_h <= 1 ||
> > > > > > > -                ast->coded_framesize * sub_packet_h > (2 +
> > > > > > > (sub_packet_h & 1)) * ast->audio_framesize)
> > > > > > > +                ast->coded_framesize * (uint64_t)sub_packet_h > (2
> > > > > > > + (sub_packet_h & 1)) * ast->audio_framesize)
> > > > > > 
> > > > > > This check seems superfluous with the one below right after it.
> > > > > > ast->coded_framesize * sub_packet_h must be equal to 2 *
> > > > > > ast->audio_framesize. It can be removed.
> > > > > > 
> > > > > > >                     return AVERROR_INVALIDDATA;
> > > > > > > -            if (ast->coded_framesize * sub_packet_h !=
> > > > > > > 2*ast->audio_framesize) {
> > > > > > > +            if (ast->coded_framesize * (uint64_t)sub_packet_h !=
> > > > > > > 2*ast->audio_framesize) {
> > > > > > >                     avpriv_request_sample(s, "mismatching interleaver
> > > > > > > parameters");
> > > > > > >                     return AVERROR_INVALIDDATA;
> > > > > > >                 }
> > > > > > 
> > > > > > How about something like
> > > > > > 
> > > > > > > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > > > > > > index fc3bff4859..09880ee3fe 100644
> > > > > > > --- a/libavformat/rmdec.c
> > > > > > > +++ b/libavformat/rmdec.c
> > > > > > > @@ -269,7 +269,7 @@ static int
> > > > > > > rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > > > > > >            case DEINT_ID_INT4:
> > > > > > >                if (ast->coded_framesize > ast->audio_framesize ||
> > > > > > >                    sub_packet_h <= 1 ||
> > > > > > > -                ast->coded_framesize * sub_packet_h > (2 +
> > > > > > > (sub_packet_h & 1)) * ast->audio_framesize)
> > > > > > > +                ast->audio_framesize > INT_MAX / sub_packet_h)
> > > > > > >                    return AVERROR_INVALIDDATA;
> > > > > > >                if (ast->coded_framesize * sub_packet_h !=
> > > > > > > 2*ast->audio_framesize) {
> > > > > > >                    avpriv_request_sample(s, "mismatching interleaver
> > > > > > > parameters");
> > > > > > 
> > > > > > Instead?
> > > > > 
> > > > > The 2 if() execute different things, the 2nd requests a
> > > > > sample, the first
> > > > > not. I think this suggestion would change when we request a sample
> > > > 
> > > > Why are we returning INVALIDDATA after requesting a sample, for that
> > > > matter? If it's considered an invalid scenario, do we really
> > > > need a sample?
> > > > 
> > > > In any case, if you don't want more files where "ast->coded_framesize *
> > > > sub_packet_h != 2*ast->audio_framesize" would print a sample request,
> > > > then maybe something like the following could be used instead?
> > > > 
> > > > > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > > > > index fc3bff4859..10c1699a81 100644
> > > > > --- a/libavformat/rmdec.c
> > > > > +++ b/libavformat/rmdec.c
> > > > > @@ -269,6 +269,7 @@ static int
> > > > > rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > > > >           case DEINT_ID_INT4:
> > > > >               if (ast->coded_framesize > ast->audio_framesize ||
> > > > >                   sub_packet_h <= 1 ||
> > > > > +                ast->audio_framesize > INT_MAX / sub_packet_h ||
> > > > >                   ast->coded_framesize * sub_packet_h > (2 +
> > > > > (sub_packet_h & 1)) * ast->audio_framesize)
> > > > >                   return AVERROR_INVALIDDATA;
> > > > >               if (ast->coded_framesize * sub_packet_h !=
> > > > > 2*ast->audio_framesize) {
> > > > > @@ -278,12 +279,16 @@ static int
> > > > > rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > > > >               break;
> > > > >           case DEINT_ID_GENR:
> > > > >               if (ast->sub_packet_size <= 0 ||
> > > > > +                ast->audio_framesize > INT_MAX / sub_packet_h ||
> > > > >                   ast->sub_packet_size > ast->audio_framesize)
> > > > >                   return AVERROR_INVALIDDATA;
> > > > >               if (ast->audio_framesize % ast->sub_packet_size)
> > > > >                   return AVERROR_INVALIDDATA;
> > > > >               break;
> > > > >           case DEINT_ID_SIPR:
> > > > > +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
> > > 
> > > sub_packet_h has not been checked for being != 0 here and in the
> > > DEINT_ID_GENR codepath.
> > 
> > Ah, good catch. This also means av_new_packet() is potentially being
> > called with 0 as size for these two codepaths.
> > 
> > > 
> > > > > +                return AVERROR_INVALIDDATA;
> > > > > +            break;
> > > > >           case DEINT_ID_INT0:
> > > > >           case DEINT_ID_VBRS:
> > > > >           case DEINT_ID_VBRF:
> > > > > @@ -296,7 +301,6 @@ static int
> > > > > rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > > > >               ast->deint_id == DEINT_ID_GENR ||
> > > > >               ast->deint_id == DEINT_ID_SIPR) {
> > > > >               if (st->codecpar->block_align <= 0 ||
> > > > > -                ast->audio_framesize * (uint64_t)sub_packet_h >
> > > > > (unsigned)INT_MAX ||
> > > > >                   ast->audio_framesize * sub_packet_h <
> > > > > st->codecpar->block_align)
> > > > >                   return AVERROR_INVALIDDATA;
> > > > >               if (av_new_packet(&ast->pkt, ast->audio_framesize *
> > > > > sub_packet_h) < 0)
> > > > 
> > > > Same amount of checks for all three deint ids, and no integer casting to
> > > > prevent overflows.
> > > 
> > > Since when is a division better than casting to 64bits to perform a
> > > multiplication?
> > 
> > This is done in plenty of places across the codebase to catch the same
> > kind of overflows. Does it make any measurable difference even worth
> > mentioning, especially considering this is read in the header?
> > 
> > All these casts make the code really ugly and harder to read. Especially
> > things like (unsigned)INT_MAX. So if there are cleaner solutions, they
> > should be used if possible.
> > Code needs to not only work, but also be maintainable.
> 
> Another option is to just change the type of the RMStream fields, like so:
> 
> > diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > index fc3bff4859..304984d2b0 100644
> > --- a/libavformat/rmdec.c
> > +++ b/libavformat/rmdec.c
> > @@ -50,8 +50,8 @@ struct RMStream {
> >      /// Audio descrambling matrix parameters
> >      int64_t audiotimestamp; ///< Audio packet timestamp
> >      int sub_packet_cnt; // Subpacket counter, used while reading
> > -    int sub_packet_size, sub_packet_h, coded_framesize; ///< Descrambling parameters from container
> > -    int audio_framesize; /// Audio frame size from container
> > +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///< Descrambling parameters from container
> > +    unsigned audio_framesize; /// Audio frame size from container
> >      int sub_packet_lengths[16]; /// Length of each subpacket
> >      int32_t deint_id;  ///< deinterleaver used in audio stream
> >  };

using unsigned would prevent the detection from overflow bugs in the form
of undefined behavior. This may make maintaince harder, this is not DSP code
where overflows would not matter much. Instead with size stuff overflows
often mean out of array later.
leaving things signed would allow early overflow detection.
But if people prefer i can send a patch that changes them to unsigned

Thanks

[...]

On Sat, Jul 10, 2021 at 03:31:14PM +0200, Michael Niedermayer wrote:
> On Sat, Apr 17, 2021 at 03:12:29AM +0200, Andreas Rheinhardt wrote:
> > James Almer:
> > > On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
> > >> James Almer:
> > >>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
> > >>>> James Almer:
> > >>>>> On 4/16/2021 7:45 PM, James Almer wrote:
> > >>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
> > >>>>>>> James Almer:
> > >>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
> > >>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
> > >>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
> > >>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
> > >>>>>>>>>>> cannot
> > >>>>>>>>>>> be represented in type 'int'
> > >>>>>>>>>>> Fixes:
> > >>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> Found-by: continuous fuzzing process
> > >>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > >>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>       libavformat/rmdec.c | 4 ++--
> > >>>>>>>>>>>       1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>>>>>>>>>
> > >>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > >>>>>>>>>>> index fc3bff4859..af032ed90a 100644
> > >>>>>>>>>>> --- a/libavformat/rmdec.c
> > >>>>>>>>>>> +++ b/libavformat/rmdec.c
> > >>>>>>>>>>> @@ -269,9 +269,9 @@ static int
> > >>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>>>>>>               case DEINT_ID_INT4:
> > >>>>>>>>>>>                   if (ast->coded_framesize >
> > >>>>>>>>>>> ast->audio_framesize ||
> > >>>>>>>>>>>                       sub_packet_h <= 1 ||
> > >>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
> > >>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
> > >>>>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h
> > >>>>>>>>>>>> (2
> > >>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
> > >>>>>>>>>>
> > >>>>>>>>>> This check seems superfluous with the one below right after it.
> > >>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
> > >>>>>>>>>> ast->audio_framesize. It can be removed.
> > >>>>>>>>>>
> > >>>>>>>>>>>                       return AVERROR_INVALIDDATA;
> > >>>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
> > >>>>>>>>>>> 2*ast->audio_framesize) {
> > >>>>>>>>>>> +            if (ast->coded_framesize *
> > >>>>>>>>>>> (uint64_t)sub_packet_h !=
> > >>>>>>>>>>> 2*ast->audio_framesize) {
> > >>>>>>>>>>>                       avpriv_request_sample(s, "mismatching
> > >>>>>>>>>>> interleaver
> > >>>>>>>>>>> parameters");
> > >>>>>>>>>>>                       return AVERROR_INVALIDDATA;
> > >>>>>>>>>>>                   }
> > >>>>>>>>>>
> > >>>>>>>>>> How about something like
> > >>>>>>>>>>
> > >>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > >>>>>>>>>>> index fc3bff4859..09880ee3fe 100644
> > >>>>>>>>>>> --- a/libavformat/rmdec.c
> > >>>>>>>>>>> +++ b/libavformat/rmdec.c
> > >>>>>>>>>>> @@ -269,7 +269,7 @@ static int
> > >>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>>>>>>              case DEINT_ID_INT4:
> > >>>>>>>>>>>                  if (ast->coded_framesize >
> > >>>>>>>>>>> ast->audio_framesize ||
> > >>>>>>>>>>>                      sub_packet_h <= 1 ||
> > >>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
> > >>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
> > >>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
> > >>>>>>>>>>>                      return AVERROR_INVALIDDATA;
> > >>>>>>>>>>>                  if (ast->coded_framesize * sub_packet_h !=
> > >>>>>>>>>>> 2*ast->audio_framesize) {
> > >>>>>>>>>>>                      avpriv_request_sample(s, "mismatching
> > >>>>>>>>>>> interleaver
> > >>>>>>>>>>> parameters");
> > >>>>>>>>>>
> > >>>>>>>>>> Instead?
> > >>>>>>>>>
> > >>>>>>>>> The 2 if() execute different things, the 2nd requests a sample,
> > >>>>>>>>> the
> > >>>>>>>>> first
> > >>>>>>>>> not. I think this suggestion would change when we request a sample
> > >>>>>>>>
> > >>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for
> > >>>>>>>> that
> > >>>>>>>> matter? If it's considered an invalid scenario, do we really need a
> > >>>>>>>> sample?
> > >>>>>>>>
> > >>>>>>>> In any case, if you don't want more files where
> > >>>>>>>> "ast->coded_framesize *
> > >>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample
> > >>>>>>>> request,
> > >>>>>>>> then maybe something like the following could be used instead?
> > >>>>>>>>
> > >>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > >>>>>>>>> index fc3bff4859..10c1699a81 100644
> > >>>>>>>>> --- a/libavformat/rmdec.c
> > >>>>>>>>> +++ b/libavformat/rmdec.c
> > >>>>>>>>> @@ -269,6 +269,7 @@ static int
> > >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>>>>             case DEINT_ID_INT4:
> > >>>>>>>>>                 if (ast->coded_framesize > ast->audio_framesize ||
> > >>>>>>>>>                     sub_packet_h <= 1 ||
> > >>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
> > >>>>>>>>>                     ast->coded_framesize * sub_packet_h > (2 +
> > >>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
> > >>>>>>>>>                     return AVERROR_INVALIDDATA;
> > >>>>>>>>>                 if (ast->coded_framesize * sub_packet_h !=
> > >>>>>>>>> 2*ast->audio_framesize) {
> > >>>>>>>>> @@ -278,12 +279,16 @@ static int
> > >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>>>>                 break;
> > >>>>>>>>>             case DEINT_ID_GENR:
> > >>>>>>>>>                 if (ast->sub_packet_size <= 0 ||
> > >>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
> > >>>>>>>>>                     ast->sub_packet_size > ast->audio_framesize)
> > >>>>>>>>>                     return AVERROR_INVALIDDATA;
> > >>>>>>>>>                 if (ast->audio_framesize % ast->sub_packet_size)
> > >>>>>>>>>                     return AVERROR_INVALIDDATA;
> > >>>>>>>>>                 break;
> > >>>>>>>>>             case DEINT_ID_SIPR:
> > >>>>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
> > >>>>>>>
> > >>>>>>> sub_packet_h has not been checked for being != 0 here and in the
> > >>>>>>> DEINT_ID_GENR codepath.
> > >>>>>>
> > >>>>>> Ah, good catch. This also means av_new_packet() is potentially being
> > >>>>>> called with 0 as size for these two codepaths.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>> +                return AVERROR_INVALIDDATA;
> > >>>>>>>>> +            break;
> > >>>>>>>>>             case DEINT_ID_INT0:
> > >>>>>>>>>             case DEINT_ID_VBRS:
> > >>>>>>>>>             case DEINT_ID_VBRF:
> > >>>>>>>>> @@ -296,7 +301,6 @@ static int
> > >>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>>>>                 ast->deint_id == DEINT_ID_GENR ||
> > >>>>>>>>>                 ast->deint_id == DEINT_ID_SIPR) {
> > >>>>>>>>>                 if (st->codecpar->block_align <= 0 ||
> > >>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
> > >>>>>>>>> (unsigned)INT_MAX ||
> > >>>>>>>>>                     ast->audio_framesize * sub_packet_h <
> > >>>>>>>>> st->codecpar->block_align)
> > >>>>>>>>>                     return AVERROR_INVALIDDATA;
> > >>>>>>>>>                 if (av_new_packet(&ast->pkt,
> > >>>>>>>>> ast->audio_framesize *
> > >>>>>>>>> sub_packet_h) < 0)
> > >>>>>>>>
> > >>>>>>>> Same amount of checks for all three deint ids, and no integer
> > >>>>>>>> casting to
> > >>>>>>>> prevent overflows.
> > >>>>>>>
> > >>>>>>> Since when is a division better than casting to 64bits to perform a
> > >>>>>>> multiplication?
> > >>>>>>
> > >>>>>> This is done in plenty of places across the codebase to catch the
> > >>>>>> same
> > >>>>>> kind of overflows. Does it make any measurable difference even worth
> > >>>>>> mentioning, especially considering this is read in the header?
> > >>>>>>
> > >>>>>> All these casts make the code really ugly and harder to read.
> > >>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
> > >>>>>> solutions, they should be used if possible.
> > >>>>>> Code needs to not only work, but also be maintainable.
> > >>>>>
> > >>>>> Another option is to just change the type of the RMStream fields,
> > >>>>> like so:
> > >>>>>
> > >>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> > >>>>>> index fc3bff4859..304984d2b0 100644
> > >>>>>> --- a/libavformat/rmdec.c
> > >>>>>> +++ b/libavformat/rmdec.c
> > >>>>>> @@ -50,8 +50,8 @@ struct RMStream {
> > >>>>>>        /// Audio descrambling matrix parameters
> > >>>>>>        int64_t audiotimestamp; ///< Audio packet timestamp
> > >>>>>>        int sub_packet_cnt; // Subpacket counter, used while reading
> > >>>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
> > >>>>>> Descrambling parameters from container
> > >>>>>> -    int audio_framesize; /// Audio frame size from container
> > >>>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
> > >>>>>> Descrambling parameters from container
> > >>>>>> +    unsigned audio_framesize; /// Audio frame size from container
> > >>>>>>        int sub_packet_lengths[16]; /// Length of each subpacket
> > >>>>>>        int32_t deint_id;  ///< deinterleaver used in audio stream
> > >>>>>>    };
> > >>>>>> @@ -277,7 +277,7 @@ static int
> > >>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>                }
> > >>>>>>                break;
> > >>>>>>            case DEINT_ID_GENR:
> > >>>>>> -            if (ast->sub_packet_size <= 0 ||
> > >>>>>> +            if (!ast->sub_packet_size ||
> > >>>>>>                    ast->sub_packet_size > ast->audio_framesize)
> > >>>>>>                    return AVERROR_INVALIDDATA;
> > >>>>>>                if (ast->audio_framesize % ast->sub_packet_size)
> > >>>>>> @@ -296,7 +296,7 @@ static int
> > >>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
> > >>>>>>                ast->deint_id == DEINT_ID_GENR ||
> > >>>>>>                ast->deint_id == DEINT_ID_SIPR) {
> > >>>>>>                if (st->codecpar->block_align <= 0 ||
> > >>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
> > >>>>>> (unsigned)INT_MAX ||
> > >>>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
> > >>>>>>                    ast->audio_framesize * sub_packet_h <
> > >>>>>> st->codecpar->block_align)
> > >>>>>>                    return AVERROR_INVALIDDATA;
> > >>>>>>                if (av_new_packet(&ast->pkt, ast->audio_framesize *
> > >>>>>> sub_packet_h) < 0)
> > >>>>>
> > >>>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
> > >>>>> so unless I'm missing something, this should be enough.
> > >>>>
> > >>>> In the multiplication ast->coded_framesize * sub_packet_h the first is
> > >>>> read via av_rb32(). Your patch will indeed eliminate the undefined
> > >>>> behaviour (because unsigned), but it might be that the check will now
> > >>>> not trigger when it should trigger because only the lower 32bits are
> > >>>> compared.
> > >>>
> > >>> ast->coded_framesize is guaranteed to be less than or equal to
> > >>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
> > >>>
> > >>
> > >> True (apart from the bound being UINT16_MAX).
> > > 
> > > Yes, my bad.
> > > 
> > >  Doesn't fix the
> > >> uninitialized data that I mentioned though.
> > >> Yet there is a check for coded_framesize being < 0 immediately after it
> > >> is read. Said check would be moot with your changes. The problem is that
> > >> if its value is not representable as an int, one could set a negative
> > >> block_align value based upon it.
> > > 
> > > With coded_framesize being an int (local variable where the value is
> > > read with avio_rb32()) and ast->coded_framesize being unsigned (context
> > > variable where the value is ultimately stored), the end result after the
> > > < 0 check will be that ast->coded_framesize is at most INT_MAX right
> > > from the beginning, so block_align can't be negative either.
> > 
> > True, the check uses a local int variable.
> 
> The issue that started this thread is still open. And even after re-reading
> this thread iam not sure what changes to it exactly are requested.
> 

> Do you or James remember what exactly you wanted me to do instead of my
> initial patch ?

ping


[...]

On 9/14/2021 6:09 PM, Michael Niedermayer wrote:
> On Sat, Jul 10, 2021 at 03:31:14PM +0200, Michael Niedermayer wrote:
>> On Sat, Apr 17, 2021 at 03:12:29AM +0200, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 4/16/2021 7:45 PM, James Almer wrote:
>>>>>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>>>>>>>> James Almer:
>>>>>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
>>>>>>>>>>>>>> cannot
>>>>>>>>>>>>>> be represented in type 'int'
>>>>>>>>>>>>>> Fixes:
>>>>>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>        libavformat/rmdec.c | 4 ++--
>>>>>>>>>>>>>>        1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>>>                case DEINT_ID_INT4:
>>>>>>>>>>>>>>                    if (ast->coded_framesize >
>>>>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>>>>                        sub_packet_h <= 1 ||
>>>>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>> +                ast->coded_framesize * (uint64_t)sub_packet_h
>>>>>>>>>>>>>>> (2
>>>>>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>
>>>>>>>>>>>>> This check seems superfluous with the one below right after it.
>>>>>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>                        return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>> +            if (ast->coded_framesize *
>>>>>>>>>>>>>> (uint64_t)sub_packet_h !=
>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>>                        avpriv_request_sample(s, "mismatching
>>>>>>>>>>>>>> interleaver
>>>>>>>>>>>>>> parameters");
>>>>>>>>>>>>>>                        return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>>                    }
>>>>>>>>>>>>>
>>>>>>>>>>>>> How about something like
>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>>>               case DEINT_ID_INT4:
>>>>>>>>>>>>>>                   if (ast->coded_framesize >
>>>>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>>>>                       sub_packet_h <= 1 ||
>>>>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>>                   if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>>                       avpriv_request_sample(s, "mismatching
>>>>>>>>>>>>>> interleaver
>>>>>>>>>>>>>> parameters");
>>>>>>>>>>>>>
>>>>>>>>>>>>> Instead?
>>>>>>>>>>>>
>>>>>>>>>>>> The 2 if() execute different things, the 2nd requests a sample,
>>>>>>>>>>>> the
>>>>>>>>>>>> first
>>>>>>>>>>>> not. I think this suggestion would change when we request a sample
>>>>>>>>>>>
>>>>>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for
>>>>>>>>>>> that
>>>>>>>>>>> matter? If it's considered an invalid scenario, do we really need a
>>>>>>>>>>> sample?
>>>>>>>>>>>
>>>>>>>>>>> In any case, if you don't want more files where
>>>>>>>>>>> "ast->coded_framesize *
>>>>>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample
>>>>>>>>>>> request,
>>>>>>>>>>> then maybe something like the following could be used instead?
>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>>>>>                  if (ast->coded_framesize > ast->audio_framesize ||
>>>>>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>>>>>                      ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>                  if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>                  break;
>>>>>>>>>>>>              case DEINT_ID_GENR:
>>>>>>>>>>>>                  if (ast->sub_packet_size <= 0 ||
>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX / sub_packet_h ||
>>>>>>>>>>>>                      ast->sub_packet_size > ast->audio_framesize)
>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>                  if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>                  break;
>>>>>>>>>>>>              case DEINT_ID_SIPR:
>>>>>>>>>>>> +            if (ast->audio_framesize > INT_MAX / sub_packet_h)
>>>>>>>>>>
>>>>>>>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>>>>>>>> DEINT_ID_GENR codepath.
>>>>>>>>>
>>>>>>>>> Ah, good catch. This also means av_new_packet() is potentially being
>>>>>>>>> called with 0 as size for these two codepaths.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>>>>>>> +            break;
>>>>>>>>>>>>              case DEINT_ID_INT0:
>>>>>>>>>>>>              case DEINT_ID_VBRS:
>>>>>>>>>>>>              case DEINT_ID_VBRF:
>>>>>>>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>                  ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>>>>>                  ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>>>>>                  if (st->codecpar->block_align <= 0 ||
>>>>>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>>>>>                      ast->audio_framesize * sub_packet_h <
>>>>>>>>>>>> st->codecpar->block_align)
>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>                  if (av_new_packet(&ast->pkt,
>>>>>>>>>>>> ast->audio_framesize *
>>>>>>>>>>>> sub_packet_h) < 0)
>>>>>>>>>>>
>>>>>>>>>>> Same amount of checks for all three deint ids, and no integer
>>>>>>>>>>> casting to
>>>>>>>>>>> prevent overflows.
>>>>>>>>>>
>>>>>>>>>> Since when is a division better than casting to 64bits to perform a
>>>>>>>>>> multiplication?
>>>>>>>>>
>>>>>>>>> This is done in plenty of places across the codebase to catch the
>>>>>>>>> same
>>>>>>>>> kind of overflows. Does it make any measurable difference even worth
>>>>>>>>> mentioning, especially considering this is read in the header?
>>>>>>>>>
>>>>>>>>> All these casts make the code really ugly and harder to read.
>>>>>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>>>>>>>> solutions, they should be used if possible.
>>>>>>>>> Code needs to not only work, but also be maintainable.
>>>>>>>>
>>>>>>>> Another option is to just change the type of the RMStream fields,
>>>>>>>> like so:
>>>>>>>>
>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>> index fc3bff4859..304984d2b0 100644
>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>>>>>>>         /// Audio descrambling matrix parameters
>>>>>>>>>         int64_t audiotimestamp; ///< Audio packet timestamp
>>>>>>>>>         int sub_packet_cnt; // Subpacket counter, used while reading
>>>>>>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>>>>>> Descrambling parameters from container
>>>>>>>>> -    int audio_framesize; /// Audio frame size from container
>>>>>>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>>>>>> Descrambling parameters from container
>>>>>>>>> +    unsigned audio_framesize; /// Audio frame size from container
>>>>>>>>>         int sub_packet_lengths[16]; /// Length of each subpacket
>>>>>>>>>         int32_t deint_id;  ///< deinterleaver used in audio stream
>>>>>>>>>     };
>>>>>>>>> @@ -277,7 +277,7 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>                 }
>>>>>>>>>                 break;
>>>>>>>>>             case DEINT_ID_GENR:
>>>>>>>>> -            if (ast->sub_packet_size <= 0 ||
>>>>>>>>> +            if (!ast->sub_packet_size ||
>>>>>>>>>                     ast->sub_packet_size > ast->audio_framesize)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>>> @@ -296,7 +296,7 @@ static int
>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>                 ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>>                 ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>>                 if (st->codecpar->block_align <= 0 ||
>>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>>>>>>>                     ast->audio_framesize * sub_packet_h <
>>>>>>>>> st->codecpar->block_align)
>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>                 if (av_new_packet(&ast->pkt, ast->audio_framesize *
>>>>>>>>> sub_packet_h) < 0)
>>>>>>>>
>>>>>>>> ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
>>>>>>>> so unless I'm missing something, this should be enough.
>>>>>>>
>>>>>>> In the multiplication ast->coded_framesize * sub_packet_h the first is
>>>>>>> read via av_rb32(). Your patch will indeed eliminate the undefined
>>>>>>> behaviour (because unsigned), but it might be that the check will now
>>>>>>> not trigger when it should trigger because only the lower 32bits are
>>>>>>> compared.
>>>>>>
>>>>>> ast->coded_framesize is guaranteed to be less than or equal to
>>>>>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
>>>>>>
>>>>>
>>>>> True (apart from the bound being UINT16_MAX).
>>>>
>>>> Yes, my bad.
>>>>
>>>>   Doesn't fix the
>>>>> uninitialized data that I mentioned though.
>>>>> Yet there is a check for coded_framesize being < 0 immediately after it
>>>>> is read. Said check would be moot with your changes. The problem is that
>>>>> if its value is not representable as an int, one could set a negative
>>>>> block_align value based upon it.
>>>>
>>>> With coded_framesize being an int (local variable where the value is
>>>> read with avio_rb32()) and ast->coded_framesize being unsigned (context
>>>> variable where the value is ultimately stored), the end result after the
>>>> < 0 check will be that ast->coded_framesize is at most INT_MAX right
>>>> from the beginning, so block_align can't be negative either.
>>>
>>> True, the check uses a local int variable.
>>
>> The issue that started this thread is still open. And even after re-reading
>> this thread iam not sure what changes to it exactly are requested.
>>
> 
>> Do you or James remember what exactly you wanted me to do instead of my
>> initial patch ?
> 
> ping

Just push your version. I think i suggested to just change the type of 
some variables to unsigned plus some extra checks, but it may not be 
worth the extra complexity.

James Almer:
> On 9/14/2021 6:09 PM, Michael Niedermayer wrote:
>> On Sat, Jul 10, 2021 at 03:31:14PM +0200, Michael Niedermayer wrote:
>>> On Sat, Apr 17, 2021 at 03:12:29AM +0200, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
>>>>>>>> James Almer:
>>>>>>>>> On 4/16/2021 7:45 PM, James Almer wrote:
>>>>>>>>>> On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
>>>>>>>>>>> James Almer:
>>>>>>>>>>>> On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
>>>>>>>>>>>>> On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
>>>>>>>>>>>>>> On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
>>>>>>>>>>>>>>> Fixes: runtime error: signed integer overflow: 65312 * 65535
>>>>>>>>>>>>>>> cannot
>>>>>>>>>>>>>>> be represented in type 'int'
>>>>>>>>>>>>>>> Fixes:
>>>>>>>>>>>>>>> 32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>        libavformat/rmdec.c | 4 ++--
>>>>>>>>>>>>>>>        1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>>>>> index fc3bff4859..af032ed90a 100644
>>>>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>>>>> @@ -269,9 +269,9 @@ static int
>>>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext
>>>>>>>>>>>>>>> *pb,
>>>>>>>>>>>>>>>                case DEINT_ID_INT4:
>>>>>>>>>>>>>>>                    if (ast->coded_framesize >
>>>>>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>>>>>                        sub_packet_h <= 1 ||
>>>>>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>>> +                ast->coded_framesize *
>>>>>>>>>>>>>>> (uint64_t)sub_packet_h
>>>>>>>>>>>>>>>> (2
>>>>>>>>>>>>>>> + (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This check seems superfluous with the one below right
>>>>>>>>>>>>>> after it.
>>>>>>>>>>>>>> ast->coded_framesize * sub_packet_h must be equal to 2 *
>>>>>>>>>>>>>> ast->audio_framesize. It can be removed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>                        return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>>> -            if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>>> +            if (ast->coded_framesize *
>>>>>>>>>>>>>>> (uint64_t)sub_packet_h !=
>>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>>>                        avpriv_request_sample(s, "mismatching
>>>>>>>>>>>>>>> interleaver
>>>>>>>>>>>>>>> parameters");
>>>>>>>>>>>>>>>                        return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>>>                    }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> How about something like
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>>>>> index fc3bff4859..09880ee3fe 100644
>>>>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>>>>> @@ -269,7 +269,7 @@ static int
>>>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext
>>>>>>>>>>>>>>> *pb,
>>>>>>>>>>>>>>>               case DEINT_ID_INT4:
>>>>>>>>>>>>>>>                   if (ast->coded_framesize >
>>>>>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>>>>>                       sub_packet_h <= 1 ||
>>>>>>>>>>>>>>> -                ast->coded_framesize * sub_packet_h > (2 +
>>>>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX /
>>>>>>>>>>>>>>> sub_packet_h)
>>>>>>>>>>>>>>>                       return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>>>                   if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>>>>                       avpriv_request_sample(s, "mismatching
>>>>>>>>>>>>>>> interleaver
>>>>>>>>>>>>>>> parameters");
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Instead?
>>>>>>>>>>>>>
>>>>>>>>>>>>> The 2 if() execute different things, the 2nd requests a
>>>>>>>>>>>>> sample,
>>>>>>>>>>>>> the
>>>>>>>>>>>>> first
>>>>>>>>>>>>> not. I think this suggestion would change when we request a
>>>>>>>>>>>>> sample
>>>>>>>>>>>>
>>>>>>>>>>>> Why are we returning INVALIDDATA after requesting a sample, for
>>>>>>>>>>>> that
>>>>>>>>>>>> matter? If it's considered an invalid scenario, do we really
>>>>>>>>>>>> need a
>>>>>>>>>>>> sample?
>>>>>>>>>>>>
>>>>>>>>>>>> In any case, if you don't want more files where
>>>>>>>>>>>> "ast->coded_framesize *
>>>>>>>>>>>> sub_packet_h != 2*ast->audio_framesize" would print a sample
>>>>>>>>>>>> request,
>>>>>>>>>>>> then maybe something like the following could be used instead?
>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>>>>> index fc3bff4859..10c1699a81 100644
>>>>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>>>>> @@ -269,6 +269,7 @@ static int
>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>>              case DEINT_ID_INT4:
>>>>>>>>>>>>>                  if (ast->coded_framesize >
>>>>>>>>>>>>> ast->audio_framesize ||
>>>>>>>>>>>>>                      sub_packet_h <= 1 ||
>>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX /
>>>>>>>>>>>>> sub_packet_h ||
>>>>>>>>>>>>>                      ast->coded_framesize * sub_packet_h >
>>>>>>>>>>>>> (2 +
>>>>>>>>>>>>> (sub_packet_h & 1)) * ast->audio_framesize)
>>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>                  if (ast->coded_framesize * sub_packet_h !=
>>>>>>>>>>>>> 2*ast->audio_framesize) {
>>>>>>>>>>>>> @@ -278,12 +279,16 @@ static int
>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>>                  break;
>>>>>>>>>>>>>              case DEINT_ID_GENR:
>>>>>>>>>>>>>                  if (ast->sub_packet_size <= 0 ||
>>>>>>>>>>>>> +                ast->audio_framesize > INT_MAX /
>>>>>>>>>>>>> sub_packet_h ||
>>>>>>>>>>>>>                      ast->sub_packet_size >
>>>>>>>>>>>>> ast->audio_framesize)
>>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>                  if (ast->audio_framesize %
>>>>>>>>>>>>> ast->sub_packet_size)
>>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>                  break;
>>>>>>>>>>>>>              case DEINT_ID_SIPR:
>>>>>>>>>>>>> +            if (ast->audio_framesize > INT_MAX /
>>>>>>>>>>>>> sub_packet_h)
>>>>>>>>>>>
>>>>>>>>>>> sub_packet_h has not been checked for being != 0 here and in the
>>>>>>>>>>> DEINT_ID_GENR codepath.
>>>>>>>>>>
>>>>>>>>>> Ah, good catch. This also means av_new_packet() is potentially
>>>>>>>>>> being
>>>>>>>>>> called with 0 as size for these two codepaths.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>> +                return AVERROR_INVALIDDATA;
>>>>>>>>>>>>> +            break;
>>>>>>>>>>>>>              case DEINT_ID_INT0:
>>>>>>>>>>>>>              case DEINT_ID_VBRS:
>>>>>>>>>>>>>              case DEINT_ID_VBRF:
>>>>>>>>>>>>> @@ -296,7 +301,6 @@ static int
>>>>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>>>>                  ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>>>>>>                  ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>>>>>>                  if (st->codecpar->block_align <= 0 ||
>>>>>>>>>>>>> -                ast->audio_framesize *
>>>>>>>>>>>>> (uint64_t)sub_packet_h >
>>>>>>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>>>>>>                      ast->audio_framesize * sub_packet_h <
>>>>>>>>>>>>> st->codecpar->block_align)
>>>>>>>>>>>>>                      return AVERROR_INVALIDDATA;
>>>>>>>>>>>>>                  if (av_new_packet(&ast->pkt,
>>>>>>>>>>>>> ast->audio_framesize *
>>>>>>>>>>>>> sub_packet_h) < 0)
>>>>>>>>>>>>
>>>>>>>>>>>> Same amount of checks for all three deint ids, and no integer
>>>>>>>>>>>> casting to
>>>>>>>>>>>> prevent overflows.
>>>>>>>>>>>
>>>>>>>>>>> Since when is a division better than casting to 64bits to
>>>>>>>>>>> perform a
>>>>>>>>>>> multiplication?
>>>>>>>>>>
>>>>>>>>>> This is done in plenty of places across the codebase to catch the
>>>>>>>>>> same
>>>>>>>>>> kind of overflows. Does it make any measurable difference even
>>>>>>>>>> worth
>>>>>>>>>> mentioning, especially considering this is read in the header?
>>>>>>>>>>
>>>>>>>>>> All these casts make the code really ugly and harder to read.
>>>>>>>>>> Especially things like (unsigned)INT_MAX. So if there are cleaner
>>>>>>>>>> solutions, they should be used if possible.
>>>>>>>>>> Code needs to not only work, but also be maintainable.
>>>>>>>>>
>>>>>>>>> Another option is to just change the type of the RMStream fields,
>>>>>>>>> like so:
>>>>>>>>>
>>>>>>>>>> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
>>>>>>>>>> index fc3bff4859..304984d2b0 100644
>>>>>>>>>> --- a/libavformat/rmdec.c
>>>>>>>>>> +++ b/libavformat/rmdec.c
>>>>>>>>>> @@ -50,8 +50,8 @@ struct RMStream {
>>>>>>>>>>         /// Audio descrambling matrix parameters
>>>>>>>>>>         int64_t audiotimestamp; ///< Audio packet timestamp
>>>>>>>>>>         int sub_packet_cnt; // Subpacket counter, used while
>>>>>>>>>> reading
>>>>>>>>>> -    int sub_packet_size, sub_packet_h, coded_framesize; ///<
>>>>>>>>>> Descrambling parameters from container
>>>>>>>>>> -    int audio_framesize; /// Audio frame size from container
>>>>>>>>>> +    unsigned sub_packet_size, sub_packet_h, coded_framesize;
>>>>>>>>>> ///<
>>>>>>>>>> Descrambling parameters from container
>>>>>>>>>> +    unsigned audio_framesize; /// Audio frame size from
>>>>>>>>>> container
>>>>>>>>>>         int sub_packet_lengths[16]; /// Length of each subpacket
>>>>>>>>>>         int32_t deint_id;  ///< deinterleaver used in audio
>>>>>>>>>> stream
>>>>>>>>>>     };
>>>>>>>>>> @@ -277,7 +277,7 @@ static int
>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>                 }
>>>>>>>>>>                 break;
>>>>>>>>>>             case DEINT_ID_GENR:
>>>>>>>>>> -            if (ast->sub_packet_size <= 0 ||
>>>>>>>>>> +            if (!ast->sub_packet_size ||
>>>>>>>>>>                     ast->sub_packet_size > ast->audio_framesize)
>>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>>                 if (ast->audio_framesize % ast->sub_packet_size)
>>>>>>>>>> @@ -296,7 +296,7 @@ static int
>>>>>>>>>> rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
>>>>>>>>>>                 ast->deint_id == DEINT_ID_GENR ||
>>>>>>>>>>                 ast->deint_id == DEINT_ID_SIPR) {
>>>>>>>>>>                 if (st->codecpar->block_align <= 0 ||
>>>>>>>>>> -                ast->audio_framesize * (uint64_t)sub_packet_h >
>>>>>>>>>> (unsigned)INT_MAX ||
>>>>>>>>>> +                ast->audio_framesize * sub_packet_h > INT_MAX ||
>>>>>>>>>>                     ast->audio_framesize * sub_packet_h <
>>>>>>>>>> st->codecpar->block_align)
>>>>>>>>>>                     return AVERROR_INVALIDDATA;
>>>>>>>>>>                 if (av_new_packet(&ast->pkt,
>>>>>>>>>> ast->audio_framesize *
>>>>>>>>>> sub_packet_h) < 0)
>>>>>>>>>
>>>>>>>>> ast->audio_framesize and sub_packet_h are never bigger than
>>>>>>>>> INT16_MAX,
>>>>>>>>> so unless I'm missing something, this should be enough.
>>>>>>>>
>>>>>>>> In the multiplication ast->coded_framesize * sub_packet_h the
>>>>>>>> first is
>>>>>>>> read via av_rb32(). Your patch will indeed eliminate the undefined
>>>>>>>> behaviour (because unsigned), but it might be that the check
>>>>>>>> will now
>>>>>>>> not trigger when it should trigger because only the lower 32bits
>>>>>>>> are
>>>>>>>> compared.
>>>>>>>
>>>>>>> ast->coded_framesize is guaranteed to be less than or equal to
>>>>>>> ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
>>>>>>>
>>>>>>
>>>>>> True (apart from the bound being UINT16_MAX).
>>>>>
>>>>> Yes, my bad.
>>>>>
>>>>>   Doesn't fix the
>>>>>> uninitialized data that I mentioned though.
>>>>>> Yet there is a check for coded_framesize being < 0 immediately
>>>>>> after it
>>>>>> is read. Said check would be moot with your changes. The problem
>>>>>> is that
>>>>>> if its value is not representable as an int, one could set a negative
>>>>>> block_align value based upon it.
>>>>>
>>>>> With coded_framesize being an int (local variable where the value is
>>>>> read with avio_rb32()) and ast->coded_framesize being unsigned
>>>>> (context
>>>>> variable where the value is ultimately stored), the end result
>>>>> after the
>>>>> < 0 check will be that ast->coded_framesize is at most INT_MAX right
>>>>> from the beginning, so block_align can't be negative either.
>>>>
>>>> True, the check uses a local int variable.
>>>
>>> The issue that started this thread is still open. And even after
>>> re-reading
>>> this thread iam not sure what changes to it exactly are requested.
>>>
>>
>>> Do you or James remember what exactly you wanted me to do instead of my
>>> initial patch ?
>>
>> ping
> 
> Just push your version. I think i suggested to just change the type of
> some variables to unsigned plus some extra checks, but it may not be
> worth the extra complexity.

+1

- Andreas