[FFmpeg-devel] [PATCH 3/4] avformat/rmdec: Use 64bit for intermediate for DEINT_ID_INT4
James Almer
jamrial at gmail.com
Wed Sep 15 00:21:11 EEST 2021
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.
More information about the ffmpeg-devel
mailing list