[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