[FFmpeg-devel] [PATCH] avformat/mov: Fix crash with too big STSZ atoms
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Sat Sep 4 00:16:04 EEST 2021
Paul B Mahol:
> On Sat, Jul 24, 2021 at 6:09 AM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
>
>> mov_read_stsz() did not ensure that every bit of a buffer is addressable
>> by an int as is required by the get_bits API, leading to a crash in
>> ticket #9344. Fix this by restricting the size more thoroughly.
>>
>> The file from said ticket will then be considered invalid; in the
>> future, we might read and process the data in chunks to actually support
>> such files.
>>
>> Fixes ticket #9344.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>> The commit message is written as if it were certain that this
>> indeed fixes the ticket, despite me not knowing it yet (as the sample
>> in question is not public).
>> The above is intended as a quick fix that is easy to backport;
>> supporting such files can be done later.
>>
>> libavformat/mov.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 3fc5a1e8ab..e0d805b07b 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2856,7 +2856,7 @@ static int mov_read_stsz(MOVContext *c, AVIOContext
>> *pb, MOVAtom atom)
>>
>> if (!entries)
>> return 0;
>> - if (entries >= (UINT_MAX - 4) / field_size)
>> + if (entries >= (INT_MAX - 4 - 8 * AV_INPUT_BUFFER_PADDING_SIZE) /
>> field_size)
>> return AVERROR_INVALIDDATA;
>> if (sc->sample_sizes)
>> av_log(c->fc, AV_LOG_WARNING, "Duplicated STSZ atom\n");
>> --
>> 2.30.2
>>
>
> Is so big bit buffer really needed?
>
No, such a big buffer is never needed, as one can read in smaller chunks
(this would actually support the files that #9344 is about). I wanted to
implement this, but I never got around to investigate what a sane chunk
size is (one that avoids reading multiple times for ordinary files). Is
it 1MB? 4MB?
> Why not check use init_get_bits8 directly and thus depends on its
> implementation directly?
>
Because then we have already allocated a buffer. Checking before the
allocation avoids that.
- Andreas
More information about the ffmpeg-devel
mailing list