[FFmpeg-devel] [PATCH 3/4] avformat/rmdec: Use 64bit for intermediate for DEINT_ID_INT4
Michael Niedermayer
michael at niedermayer.cc
Sat Jul 10 16:31:14 EEST 2021
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 ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20210710/fee29579/attachment.sig>
More information about the ffmpeg-devel
mailing list