[FFmpeg-devel] [PATCH] avformat/mov: Fix crash with too big STSZ atoms

[Andreas Rheinhardt]

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