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). 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 _______________________________________________ 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".
