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

Andreas Rheinhardt andreas.rheinhardt at outlook.com
Wed Sep 15 00:24:57 EEST 2021


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 at 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


More information about the ffmpeg-devel mailing list