Repository navigation
reject jpeg scans using an undefined quantisation table - #1135
Open
metsw24-max wants to merge 1 commit into
Open
metsw24-max wants to merge 1 commit into
metsw24-max wants to merge 1 commit into
Conversation
Neither JPEG decoder initialises its dequant tables, and nothing checks that the table a component selects in SOF was loaded by a DQT marker, so a file without that DQT was dequantised with stale heap bytes that ended up in the output pixels. Track which tables were defined and reject such a scan at scan start, as libjpeg does.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stale heap bytes decoded as pixels when a JPEG component has no DQT table
JpegContext::dequantis never initialised (the context comes fromnew JpegContextand the constructor only setsstreamandimg_n), and the Avx2jpeg__jpegismalloc'ed. Neither decoder checks that the table a component selects in SOF (tq) was actually loaded by a DQT marker, so a file that omits the DQT, or leaves out one of its tables, is dequantised with whatever the allocator left in those 512 bytes.JpegDecodeBlockmultiplies every coefficient by it (JpegFinishdoes the same for progressive files) and the IDCT output comes back fromSimdImageLoadFromMemoryas pixels.A 64x64 JPEG written by
SimdImageSaveToMemorywith its DQT segment removed still decodes on both decoders. With the context memory pre-filled with 0x00 every Gray8 pixel is 128; with 0x01 or 0xA5 the output follows the fill (Base pixel sums 532290 and 519411, Avx2 516617 and 516829). Dropping only the chroma table from a colour JPEG does the same to the chroma planes.Fix: record which tables a DQT defined (cleared with the Huffman flags at header start) and reject a scan whose components point at an undefined one, which is where libjpeg raises
JERR_NO_QUANT_TABLE. The Avx2 decoder gets the same change, as it keeps its own marker and scan-header parsers.Notes: 187 valid JPEGs (the three bundled samples, 112 baseline and progressive files written by macOS ImageIO, 72 from
SimdImageSaveToMemory) decode bit-identically before and after in all five output formats, throughSimd/Baseon arm64 andBase/Sse41/Avx2on x86_64 under Rosetta (Avx2::ImageLoadFromMemorycalled directly).Test -fi=ImageLoad -fi=ImageSavepasses. In the full-m=arun,RecursiveBilateralFilterAutoTestfails with the same 241 vs 240 mismatch on unmodified master on this machine (Apple M5, Apple clang), so I excluded it; everything else passed up to 91%, where I stopped the Gemm32f, DescrIntCosineDistancesMxNa and SynetDeconvolution32f groups after about an hour in reference code. They do not touch the loader, but a CI run would still be worth having. The check runs once per scan, so decode speed should be unaffected.