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 <mich...@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 ? thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
