[FFmpeg-devel] [PATCH] avcodec/jpegxl_parser: fix various memory issues

[Andreas Rheinhardt]

Leo Izen:
> On 10/2/23 16:40, Andreas Rheinhardt wrote:
>> Leo Izen:
>>> The spec caps the prefix alphabet size to 32768 (i.e. 1 << 15) so we
>>> need to check for that and reject alphabets that are too large.
>>
>> No, we don't "need to", we can. FFmpeg is not a validator tool.
> 
> We need to because we risk over-allocating otherwise. If the signalled
> value is far too large, we consume a pointlessly large amount of memory.
> 

Even then we do not need to do it because the spec says so, but because
it is appropriate to not allocate too much.

>>
>>>
>>> Additionally, there's no need to allocate buffers that are as large as
>>> the maximum alphabet size as these aren't stack-allocated, they're heap
>>> allocated and thus can be variable size.
>>>
>>> Added an overflow check as well, which fixes leaking the buffer, and
>>> capping the alphabet size fixes two potential overruns as well.
>>>
>>> Fixes: out of array access
>>> Fixes: 62089/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-
>>>      5437089094959104.fuzz
>>>
>>> Found-by: continuous fuzzing process
>>>      https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Found-by: Hardik Shah of Vehere (Dawn Treaders team)
>>> Co-authored-by: Michael Niedermayer <michael at niedermayer.cc>
>>> Signed-off-by: Leo Izen <leo.izen at gmail.com>
>>> ---
>>>   libavcodec/jpegxl_parser.c | 23 +++++++++++++++++------
>>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c
>>> index d25a1b6e1d..51af0f4ed1 100644
>>> --- a/libavcodec/jpegxl_parser.c
>>> +++ b/libavcodec/jpegxl_parser.c
>>> @@ -46,6 +46,8 @@
>>>   #define JXL_FLAG_USE_LF_FRAME 32
>>>   #define JXL_FLAG_SKIP_ADAPTIVE_LF_SMOOTH 128
>>>   +#define MAX_PREFIX_ALPHABET_SIZE (1u << 15)
>>> +
>>>   #define clog1p(x) (ff_log2(x) + !!(x))
>>>   #define unpack_signed(x) (((x) & 1 ? -(x)-1 : (x))/2)
>>>   #define div_ceil(x, y) (((x) - 1) / (y) + 1)
>>> @@ -724,16 +726,17 @@ static int read_vlc_prefix(GetBitContext *gb,
>>> JXLEntropyDecoder *dec, JXLSymbolD
>>>       if (ret < 0)
>>>           goto end;
>>>   -    buf = av_calloc(1, 262148); // 32768 * 8 + 4
>>> +    buf = av_calloc(1, dist->alphabet_size * (2 * sizeof(int8_t) +
>>> sizeof(int16_t) + sizeof(uint32_t))
>>> +                       + sizeof(uint32_t));
>>
>> You can avoid the multiplication by using av_calloc((2 * sizeof(int8_t)
>> + sizeof(int16_t) + sizeof(uint32_t)) + sizeof(uint32_t),
>> dist->alphabet_size).
> 
> That's not the same thing. This will cause us to overallocate by
> dist-alphabet_size - 4 bytes. Is that okay?
> 

Ok, I see it now. But then don't use av_calloc(), but av_mallocz().

- Andreas