[FFmpeg-devel] [PATCH 3/4] avformat/rmdec: Use 64bit for intermediate for DEINT_ID_INT4

Michael Niedermayer michael at niedermayer.cc
Wed Sep 15 00:09:07 EEST 2021


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


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210914/be251280/attachment.sig>


More information about the ffmpeg-devel mailing list