From e5e9d3b877c2b26dd57fe94ac18c6f9cb62c947e Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Tue, 4 Apr 2023 09:09:16 -0400 Subject: [PATCH] LibGfx: Implement hopefully correct max_symbol handling in webp decoder The spec is at best misleading here, suggesting that max_symbol should be set to "num_code_lengths" if it's not explicitly stored. But num_code_lengths doesn't mean the num_code_lengths mentioned a few lines further up in the spec, but alphabet_size! (I had to cheat and look at libwebp instead of the spec for this: See vp8l_dec.c, ReadHuffmanCode() which passes alphabet_size to ReadHuffmanCodeLengths() as num_symbols, and ReadHuffmanCodeLengths() then sets max_symbol to that.) I haven't yet found a file that uses max_symbol, so this isn't actually tested. But it's close to what's in libwebp, so maybe it works! --- .../Libraries/LibGfx/ImageFormats/WebPLoader.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp index bd8fb9864a..ecee7f7d71 100644 --- a/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp +++ b/Userland/Libraries/LibGfx/ImageFormats/WebPLoader.cpp @@ -359,11 +359,20 @@ static ErrorOr decode_webp_chunk_VP8L_prefix_code(WebPL dbgln_if(WEBP_DEBUG, " code_length_code_lengths[{}] = {}", kCodeLengthCodeOrder[i], code_length_code_lengths[kCodeLengthCodeOrder[i]]); } - int max_symbol = num_code_lengths; + // The spec is at best misleading here, suggesting that max_symbol should be set to "num_code_lengths" if it's not explicitly stored. + // But num_code_lengths doesn't mean the num_code_lengths mentioned a few lines further up in the spec (and in scope here), + // but alphabet_size! + // + // Since the spec doesn't mention it, see libwebp vp8l_dec.c, ReadHuffmanCode() + // which passes alphabet_size to ReadHuffmanCodeLengths() as num_symbols, + // and ReadHuffmanCodeLengths() then sets max_symbol to that.) + unsigned max_symbol = alphabet_size; if (TRY(bit_stream.read_bits(1))) { int length_nbits = 2 + 2 * TRY(bit_stream.read_bits(3)); max_symbol = 2 + TRY(bit_stream.read_bits(length_nbits)); dbgln_if(WEBP_DEBUG, " extended, length_nbits {} max_symbol {}", length_nbits, max_symbol); + if (max_symbol > alphabet_size) + return context.error("WebPImageDecoderPlugin: invalid max_symbol"); } auto const code_length_code = TRY(Compress::CanonicalCode::from_bytes({ code_length_code_lengths, sizeof(code_length_code_lengths) })); @@ -373,8 +382,7 @@ static ErrorOr decode_webp_chunk_VP8L_prefix_code(WebPL u8 last_non_zero = 8; // "If code 16 is used before a non-zero value has been emitted, a value of 8 is repeated." // "A prefix table is then built from code_length_code_lengths and used to read up to max_symbol code lengths." - // FIXME: what's max_symbol good for? (seems to work with alphabet_size) - while (code_lengths.size() < alphabet_size) { + while (code_lengths.size() < max_symbol) { auto symbol = TRY(code_length_code.read_symbol(bit_stream)); if (symbol < 16) {