(RJPEG) Harden rjpeg against malformed input and cut per-decode overhead (plus regression tests) - #19274
Merged
Merged
Conversation
…t input Five defects reachable from a malformed or truncated JPEG, all found with ASan/UBSan over a truncation and random-corruption sweep, plus a sample that pins each of them down. Out-of-range DC magnitude category. The category is a raw Huffman table value (0-255), and is used both as a shift count and as an index into rjpeg_bmask[17] / rjpeg_jbias[16]. The baseline path rejected only negative values and the progressive path checked nothing at all, so rjpeg_jpeg_huff_decode's -1 error return flowed straight into a negative index. A corrupt DHT therefore reads both tables out of bounds and shifts by more than the bit-buffer width. The AC paths were already safe because they mask the category with 15. Both DC sites now range check. Undefined left shifts of negative values. rjpeg_build_fast_ac shifts the signed coefficient k, and the progressive path shifts dc, the fast-AC coefficient and the extend_receive result. All four are routinely negative; the first fires on ordinary valid images. Replaced with multiplication, which is defined and generates the same code. Non-terminating marker scan. Reading a segment length at end of data yields 0, so the payload skip computes n = -2. The existing clamp only bounds the cursor from above, so the cursor walks backwards and the enclosing scan re-reads the same bytes forever. A four byte input (FF D8 FF E0) hangs both the synchronous and the iterative driver. Reject a negative payload length. samples/formats/jpeg exercises all of this: the two crafted streams carry a DHT whose first DC symbol is 0x40, and the truncation case walks every length of a valid stream to assert termination. Run it under ASan/UBSan to get the full value. Decoding of valid images is unchanged - the twelve image corpus is byte identical through both drivers.
Two hot-path costs in JPEG decoding, both measured with a sampling profile over the twelve image corpus. cpu_features_get() was re-probing on every call, and rjpeg_setup_jpeg() calls it once per decode to pick its SIMD kernels. The probe is not cheap: on x86 it issues a chain of serialising CPUID instructions, and on ARM/Linux it opens and line-parses /proc/cpuinfo once per feature queried, four times per call. Measured here at 34.0 us per call, which is most of the cost of decoding a thumbnail. The answer cannot change over the lifetime of the process, so probe once. The mask is published with a release store and consumed with an acquire load through retro_atomic.h, so a thread observing the ready flag is guaranteed to see the whole mask; the probe is idempotent, so threads that race before anyone has published simply compute the same bits. Verified race free under ThreadSanitizer with eight threads contending on the first call. The entropy bit buffer was a 32-bit accumulator topped up to within one byte of full, so a refill admitted only one or two bytes and grow_buffer was re-entered about once per coefficient - measured at 2.11 bytes per refill across the corpus, and 5.8% to 9.3% of decode time depending on bitrate. Widening it to a register-sized word carries about seven bytes per refill instead. The width follows the natural register size, so 32-bit targets keep the original accumulator rather than pay for emulated 64-bit shifts on every symbol; both widths were verified to produce identical output. The read-ahead is deliberately held at the original depth whenever a resident frontier is in play. The byte-at-a-time path probes one byte past a 0xFF to tell a marker from a stuffed literal, and a deeper target pushes that probe past the frontier, consuming bytes that have not arrived and committing an MCU row from them. Prefix decoding is bounded by byte arrival rather than by refill cost anyway. Also fixes a C++ style comment in the WEBOS branch of cpu_features_get_model_name that broke -std=c90 -pedantic-errors builds everywhere, since comments are lexed before conditionals are evaluated. Decoding is byte identical: the twelve image corpus matches through both drivers at both accumulator widths, and prefix decoding reproduces the whole-buffer output at every chunk size from one byte upwards. 64x64 0.0691 -> 0.0269 ms 2.57x 320x240 0.4947 -> 0.4088 ms 1.21x 512x700 2.3095 -> 2.0634 ms 1.12x 1920x1080 10.0172 -> 9.2731 ms 1.08x 1080p q98 17.6731 -> 15.9218 ms 1.11x 4K 34.1665 -> 31.8137 ms 1.07x
Contributor
Author
|
Just adding this in here as a reminder so I don't forget - Remaining headroom vs libjpeg-turbo is 1.09×, and it's all in the entropy decoder — where turbo is plain scalar C too, so it's reachable. The profile put rjpeg at 64% entropy / 19% IDCT / 16% output, with the shape nearly identical to turbo's; the bit-buffer widening took the easy part and what's left is the Huffman decode structure itself. Bigger job than either of these patches. |
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
1 participant
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.
Two commits against
libretro-common's JPEG decoder. The first fixes five defects reachable from a malformed or truncated file, including an infinite loop that a four byte input is enough to trigger. The second removes two per-decode costs that were dominating small-image decodes. Decoding of valid images is byte identical either way.This started as a performance audit of
rjpegagainst the other JPEG decoders in common use, and the correctness problems turned up while sanitizing the benchmark corpus. The hardening is the more important half.1. Correctness
All five were found with ASan/UBSan over a truncation and random-corruption sweep, and all five are pre-existing on current
master.Non-terminating marker scan. Reading a segment length at end of data yields 0, so the payload skip computes
n = -2. The existing clamp only bounds the cursor from above, so the cursor walks backwards and the enclosing scan re-reads the same bytes forever.FF D8 FF E0— four bytes — hangs both the synchronous and the iterative driver. Any truncated thumbnail on disk is enough to wedge a decode thread.Out-of-range DC magnitude category. The category is a raw Huffman table value (0–255) used both as a shift count and as an index into
rjpeg_bmask[17]/rjpeg_jbias[16]. The baseline path rejected only negative values; the progressive path checked nothing at all, sorjpeg_jpeg_huff_decode's-1error return flowed straight into a negative index. A corrupt DHT reads both tables out of bounds (confirmed by ASan as a global-buffer-overflow) and shifts by more than the bit-buffer width. The AC paths were already safe — they mask the category with 15.Four undefined left shifts of negative values.
rjpeg_build_fast_acshifts the signed coefficientk, and the progressive path shiftsdc, the fast-AC coefficient, and theextend_receiveresult. The first fires on ordinary valid images — an UBSan-enabled build currently reports it about 1770 times over a twelve image corpus. Replaced with multiplication, which is defined and generates the same code.Sanitizer results over ~3,200 decodes (12 image corpus, two crafted reproducers, all 819 truncation lengths of a small image, 480 truncations of the large corpus, 300 random-corruption cases, each through both drivers):
2. Performance
cpu_features_get()was re-probing on every call, andrjpeg_setup_jpeg()calls it once per decode to select its SIMD kernels. The probe is not cheap — on x86 a chain of serialisingCPUIDinstructions, and on ARM/Linux an open and line-parse of/proc/cpuinfoonce per feature queried, four times per call. Measured at 34.0 µs per call, which is most of the cost of decoding a menu thumbnail. The answer cannot change over the process lifetime, so it is now probed once and published throughretro_atomic.hwith a release store / acquire load. Verified race free under ThreadSanitizer with eight threads contending on the first call.The entropy bit buffer was a 32-bit accumulator topped up to within one byte of full, so a refill admitted only one or two bytes — measured at 2.11 bytes per refill, with
grow_bufferre-entered roughly once per coefficient and accounting for 5.8%–9.3% of decode time depending on bitrate. It now follows the natural register width, carrying about seven bytes per refill. 32-bit targets (PSP, Vita, 3DS, Wii, PS2, 32-bit ARM) keep the original accumulator rather than pay for emulated 64-bit shifts on every symbol; both widths were verified to produce identical output.Read-ahead is deliberately held at the original depth whenever a resident frontier is in play (
rjpeg_set_avail). The byte-at-a-time path probes one byte past a0xFFto tell a marker from a stuffed literal, and a deeper target would push that probe past the frontier and commit an MCU row from bytes that have not arrived.rjpeg before vs after
The small-image win matters disproportionately: the menu decodes grids of thumbnails, and there the fixed 34 µs probe was the dominant term.
3. Comparison against other JPEG decoders
Same corpus, same machine, same session. Decode to 32-bit RGBA in every case, since that is what the frontend needs. Medians over 12–200 iterations with allocation and free included; three interleaved passes per decoder, best median taken, because the benchmark host is a shared VM. Lower is better.
rjpeg's ancestor, so it is the most directly informative comparison.libjpeg.soon essentially every desktop and mobile target today. SIMD enabled, nativeJCS_EXT_RGBAoutput.Relative throughput, geometric mean across the corpus (higher is faster):
Reading of that:
rjpegwas already at parity and is now ahead by 1.16× overall, with the largest margins on 4:2:2 (1.24×) and progressive (1.35×), where the existing SIMD kernels do the most work.decode_mcuis plain scalar C — libjpeg-turbo has no SIMD Huffman decoder — so the residual gap is scalar code structure, not hand-written assembly.rjpegis 1.92× faster. jpeg-8c's deficit is architectural: IJG v7+ upsamples chroma in the DCT domain via a 16×16 scaled IDCT (confirmed by probingDCT_h_scaled_sizeat runtime), which costs roughly double on subsampled images. It buys nothing measurable — see below.Output quality
Speed is meaningless if it is bought with precision, so the decoders were also measured against a float64 reference: DCT coefficients extracted via
jpeg_read_coefficients, then dequantised, exact float64 2D IDCT, float64 triangle chroma upsampling and float64 YCbCr→RGB, with no integer intermediate stages.rjpegsits on top of libjpeg-turbo to within 0.01 dB. The jpeg-8c row against the float64 reference is not a like-for-like comparison because it targets a different reconstruction model; the model-neutral row (PSNR against the pristine pre-compression image) puts all four within 0.1 dB, which is why the DCT-domain upsampling is described above as buying nothing for its cost.The 4:2:2 row is a real difference rather than noise:
rjpegcarries a fix that upstream stb_image still has, transposing the weights on the final column of h2v1 upsampling. The divergence is confined to exactly one column (w-2), andrjpegmatches libjpeg'sh2v1_fancy_upsamplewhere stb does not.4. Testing
New sample at
samples/formats/jpeg, following thesamples/formats/jsonconvention. Test vectors are embedded as C arrays — rather than shipping a large fuzz case, the two malformed streams are minimal (~800 bytes), crafted by patching the first DC symbol of a valid image's DHT to0x40.Run it under ASan/UBSan for the full value — without the range checks, both
bad_dc_category_*cases trip them.Verification performed:
cpu_features_get()call.-std=c90 -pedantic-errorsand-std=gnu90 -pedantic-errors -Wall -Wextra. The second commit also fixes a pre-existing C++ style comment in the WEBOS branch ofcpu_features_get_model_namethat broke strict C90 builds everywhere, since comments are lexed before conditionals are evaluated.Benchmark host is a single-core Xeon VM, so absolute timings are relative-only; the ratios are what matter. Every decoder was measured in the same session with the same harness and the same output format.