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