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 <[email protected]> >>> Signed-off-by: Leo Izen <[email protected]> >>> --- >>> 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 _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
