Skip to content

Read the extra-bits tables without a bounds check, as C does - #217

Open
kunpai wants to merge 1 commit into
google:masterfrom
kunpai:extra-bits-unchecked
Open

kunpai wants to merge 1 commit into
google:masterfrom
kunpai:extra-bits-unchecked

Conversation

@kunpai

@kunpai kunpai commented Oct 28, 2026

Copy link
Copy Markdown

Read the extra-bits tables without a bounds check, as C does

Problem

ZopfliGetLengthSymbolExtraBits and ZopfliGetDistSymbolExtraBits (C, Rust) look up a symbol's extra-bit count in a fixed table. C indexes the table directly, table[s - 257] for lengths and table[s] for distances, with no bounds check. The port returns 0 for an out-of-range symbol. These are reached from ZopfliCalculateBlockSize when a store holds a command whose length symbol is below 257 (a length under 3) or whose distance symbol is 30 or 31 (a distance over 32768). For example:

# C Rust
1 a length-under-3 command (AutoType) reads the length table at a negative index and uses whatever is there uses 0 extra bits
2 a distance-over-32768 command (32-bit target) reads the 30-entry distance table at index 30/31 and gets 16/17 from the next bytes uses 0 extra bits

Question

Which behaviour do we want: C's or the Rust port's? Specifically, for a length symbol below 257 or a distance symbol of 30/31, should the extra-bits lookup read past its table (like the C source), or return 0 (like the Rust port)?

This PR implements C's behaviour.

Fix

Both functions drop the range check and index the table unchecked, so an out-of-range symbol reads past it as C's table[...] does:

-    if s < MIN_LENGTH_SYMBOL || s >= MIN_LENGTH_SYMBOL + TABLE.len() { 0 } else { TABLE[s - MIN_LENGTH_SYMBOL] }
+    unsafe { *TABLE.get_unchecked(s.wrapping_sub(MIN_LENGTH_SYMBOL)) }
-    if s >= TABLE.len() { 0 } else { TABLE[s] }
+    unsafe { *TABLE.get_unchecked(s) }

Replayed before and after (plain = x86-64, arm32 = 32-bit; ZopfliCalculateBlockSize[AutoType] returns the block size as raw f64 bits):

# C Rust main Rust patched
1, x86-64 4644372300097912832 4634274385308418048 4634274385308418048 (still differs)
1, arm32 matches the port n/a matches C exactly
2, x86-64 SIGSEGV (11) 0.0 reads out of bounds, returns 4631952216750555136
2, arm32 4621819117588971520 0.0 0.0 (reads 0 past its table)

The port now performs the same unchecked table read as C, leaving its safe return 0. But the value it reads is whatever sits in memory past its table, which is not the memory past C's table and differs by architecture, so the result generally does not match C: on x86-64 case 1 still differs, and case 2's garbage differs on both. Only on the 32-bit build does case 1 land on the same value as C. This is the residual the note below is about: the patch enters C's out-of-bounds-read class but cannot match a value that depends on each program's own layout.

(Case 2's range, lstart > lend, is only reached once the ZopfliCalculateBlockSize range guard is also removed (#215); the numbers above are with both patches applied.)

Downstream check. The normal path is untouched: ZopfliGzipCompress on the repository's README gives the same 831-byte stream from the port before and after this patch, and gzip.decompress restores the file. C's stream is also 831 bytes but differs in a few bytes, for a separate reason (#213).

Notes for review:

  • This patch reproduces C's undefined behaviour and must not be merged. It removes the bounds check and reads the extra-bits tables out of bounds, which is memory-unsafe; safe Rust returns 0. The read value is layout- and architecture-dependent (0 or 16/17 past the distance table, garbage past the length table), which is exactly why it should not ship. The PR exists only to show the divergence and ask whether these lookups should be bounds-checked, not to ship.

… does

C indexes table[s - 257] and table[s] with no check, so a length
symbol below 257 or a distance symbol of 30 or 31 reads past the table.
The port returned 0. This reproduces C's out-of-bounds read and is not
meant to be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant