enc/dec: harden integer overflow checks in size computations (defense-in-depth) by SongT-50 · Pull Request #1471 · google/brotli

SongT-50 · 2026-05-03T07:09:14Z

Summary

This PR adds defense-in-depth Integer Overflow / size_t wrap checks at five
size-computation sites in brotli's encoder and decoder. The checks are
defensive: triggering the underlying issues from a normal caller path is not
currently demonstrated. The motivation is to reduce the defense-in-depth gap
and to match the hardening style already used in surrounding code (e.g. early
returns on BROTLI_DECODER_ALLOC failures followed by buffer/length resets).

The cluster was identified by an external multi-perspective code review
(architect / defender / attacker perspectives) followed by static audit
against the current decoder/encoder caps. No exploit path was needed for
this defensive hardening.

Sites covered

#	File:Line	Issue summary	Fix style
1	`c/enc/block_encoder_inc.h:18`	`histograms_size * histogram_length_` not checked for size_t overflow before allocating `depths_` / `bits_`	Multiplication overflow guard
2	`c/dec/state.c:157`	`sizeof(HuffmanCode) * ntrees * max_table_size + htree_size` cascades arithmetic without intermediate checks	Addition + multiplication guards
3	`c/enc/block_splitter.c:59`	`from_pos + insert_len > mask` performs the addition before the bound check, so wrap can bypass the split logic	Pre-check `insert_len` against the available tail
4	`c/enc/block_splitter_inc.h:457`	`data_size * num_histograms` and `length * bitmaplen` allocated without overflow guards	Multiplication overflow guards
5	`c/enc/block_splitter_inc.h:211`	`num_blocks + 4 * HISTOGRAMS_PER_BATCH` not checked; the result is used as both an allocation size and as an offset base	Addition overflow guard

All five fixes are pure additions: when the input values would cause size_t
to wrap, the code returns early (or signals the existing out-of-memory /
format-error path that the surrounding code already handles). Behavior on
currently-reachable inputs is unchanged.

Severity / impact (honest disclosure)

These are defense-in-depth changes. We have not demonstrated a reachable
exploit path under brotli's current decoder/encoder caps:

BROTLI_LARGE_MAX_WBITS = 30 (decoder window cap; 1 GB << INT_MAX)
BROTLI_BLOCK_SIZE_CAP = 1 << 24 (16 MiB metablock cap)
SHARED_BROTLI_MIN_DICTIONARY_WORD_LENGTH = 4 (dictionary word lower bound)
kRingBufferWriteAheadSlack = 542 (transform output slack)
decoder transform_idx range checks already present in the parse path

Under those caps, the size-computation sites covered here are not reachable
from a normal caller / standard brotli stream. The sites become reachable if:

a future change relaxes one of the caps, or
a malformed metablock bypasses one of the parse-time validators, or
a caller (encoder or decoder) drives the affected internal state through
an unusual path not currently exercised.

In all three cases, the additions in this PR turn an undefined-behavior
size_t wrap into a clean early return, matching the rest of the codebase's
defensive style.

We therefore frame the changes as hardening rather than as fixes for
confirmed exploitability. CWE-190 and CWE-680 are listed because the
underlying primitives are integer overflow into allocation size.

Compatibility

The added checks are pure additions. On all currently-reachable inputs the
new checks are not triggered and behavior is identical. On inputs that
would have wrapped size_t the code now returns early (encoder: no-op
return / decoder: BROTLI_FALSE), matching how the surrounding code already
handles BROTLI_ALLOC failure.

No public API or ABI change.

Tests

Static analysis: each site mirrors the structure of an existing
overflow-aware site elsewhere in the codebase. We extracted the audit
trail from a multi-perspective review (architect / defender / attacker
perspectives) and verified each fix against the surrounding caps and
caller invariants.

Unit / harness tests: not added in this PR. The host environment used to
prepare the patch did not have a working C toolchain (no gcc / clang / cl
/ tcc on PATH), so we deferred ASan / UBSan binary verification. We're
happy to add minimal Edge-Case Test cases for each site (driven via the
encoder / decoder internal harness) in a follow-up before merge — please
let us know if maintainers prefer tests bundled in the same PR or as a
follow-up.

Honest disclosure

The audit was performed by a multi-perspective code review, not by a
fuzzing campaign. No corpus / repro input is included.
The attacker-perspective analysis classifies all 5 sites as
"boundary defense-in-depth, not currently reachable from a standard
brotli stream under the current caps".
We have intentionally avoided "exploit" / "PoC" framing: there is no
demonstrated end-to-end exploit, only an audit-level defense gap.
Reproduction steps for each site would require driving the affected
internal state (e.g. very large histograms_size × histogram_length_)
which is not reachable from caller-controlled input under the current
caps.

If maintainers prefer a different framing (e.g. split per-file, drop some
sites that are deemed unreachable even with cap relaxations, require
ASan-verified repros), we'll happily revise.

References

CWE-190 (Integer Overflow or Wraparound): https://cwe.mitre.org/data/definitions/190.html
CWE-680 (Integer Overflow to Buffer Overflow): https://cwe.mitre.org/data/definitions/680.html

…-in-depth) Add defense-in-depth size_t-overflow guards at five size-computation sites in brotli's encoder and decoder paths: - P1 c/enc/block_encoder_inc.h:18 (BuildAndStoreEntropyCodes): guard histograms_size * histogram_length_ multiplication - P2 c/dec/state.c:157 (BrotliDecoderHuffmanTreeGroupInit): guard alphabet_size_limit + 376 addition + sizeof(HuffmanCode) * ntrees * max_table_size cascade + code_size + htree_size addition - P3 c/enc/block_splitter.c:59 (CopyLiteralsToByteArray): guard from_pos + insert_len size_t wrap before > mask check - P4 c/enc/block_splitter_inc.h:457 (FindBlocks workspace): guard data_size * num_histograms and length * bitmaplen multiplications - P5 c/enc/block_splitter_inc.h:211 (ClusterBlocks workspace): guard num_blocks + 4 * HISTOGRAMS_PER_BATCH addition The cluster was identified by a multi-perspective audit (architect / defender / attacker perspectives) on the current HEAD. The 15-cell multi-perspective analysis indicates these patterns are guarded by existing decoder/encoder caps (no demonstrated reachable exploit path under the current decoder/encoder caps). Existing caps that bound the affected values under normal use: BROTLI_LARGE_MAX_WBITS = 30 (decoder window cap; 1 GB << INT_MAX) BROTLI_BLOCK_SIZE_CAP = 1 << 24 (16 MiB metablock cap) SHARED_BROTLI_MIN_DICTIONARY_WORD_LENGTH = 4 kRingBufferWriteAheadSlack = 542 decoder transform_idx range checks already present in parse path The hardening is proposed as defensive narrowing rather than a fix for a demonstrated vulnerability. All five guards are pure additions: on currently-reachable inputs the new checks are not triggered and behavior is identical; on inputs that would have wrapped size_t the code now returns early, matching the surrounding BROTLI_IS_OOM(m) / return BROTLI_FALSE pattern. CWE-190 (Integer Overflow), CWE-680 (Integer Overflow to Buffer Overflow). No public API or ABI change.

eustas · 2026-05-04T08:20:13Z

    size_t num_blocks = 0;
    const size_t bitmaplen = (num_histograms + 7) >> 3;
+    /* defense-in-depth: refuse if either multiplication wraps size_t. */
+    if (num_histograms != 0 && data_size > SIZE_MAX / num_histograms) return;


data_size is at most 704; num_histograms is at most 100

eustas · 2026-05-04T08:26:51Z

    const size_t bitmaplen = (num_histograms + 7) >> 3;
+    /* defense-in-depth: refuse if either multiplication wraps size_t. */
+    if (num_histograms != 0 && data_size > SIZE_MAX / num_histograms) return;
+    if (bitmaplen     != 0 && length    > SIZE_MAX / bitmaplen)     return;


bitmaplen is at most 13; length is at most 16M

eustas · 2026-05-04T08:31:33Z

                              uint8_t* block_ids,
                              BlockSplit* split) {
+  /* defense-in-depth: refuse if num_blocks + 4 * HISTOGRAMS_PER_BATCH wraps. */
+  if (num_blocks > SIZE_MAX - 4 * HISTOGRAMS_PER_BATCH) return;


4 * HISTOGRAMS_PER_BATCH is 256; num_blocks is at most 16M (greatly overestimated)

eustas · 2026-05-04T08:35:14Z

+       from_pos. cmds[i].insert_len_ is encoder-internal and bounded by the
+       command builder; this is a defensive narrowing for hypothetical reuse
+       with caller-controlled insert_len. */
+    if (insert_len > (size_t)0 - from_pos) {


Just return will cause catastrophic consequences.
Not speaking that both insert_len and mask below 16M, thus this situation is not possible

eustas · 2026-05-04T08:42:03Z

    size_t* storage_ix, uint8_t* storage) {
+  /* defense-in-depth: refuse if histograms_size * histogram_length_ wraps size_t */
+  if (self->histogram_length_ != 0 &&
+      histograms_size > SIZE_MAX / self->histogram_length_) {


histogram_size is at most 257; histogram_length is at most 704.

eustas · 2026-05-04T08:44:50Z

     less than 16 symbols). */
+  /* defense-in-depth: each step of the size cascade can wrap size_t under
+     hypothetical caller / metablock values that bypass the parse-time caps. */
+  if (alphabet_size_limit > SIZE_MAX - 376) return BROTLI_FALSE;


alphabet_size is at most few K.

eustas · 2026-05-04T08:45:02Z

+  if (alphabet_size_limit > SIZE_MAX - 376) return BROTLI_FALSE;
  const size_t max_table_size = alphabet_size_limit + 376;
+  if (max_table_size != 0 &&
+      ntrees > SIZE_MAX / sizeof(HuffmanCode) / max_table_size) {


eustas · 2026-05-04T08:45:54Z

+  }
  const size_t code_size = sizeof(HuffmanCode) * ntrees * max_table_size;
+  if (ntrees != 0 && ntrees > SIZE_MAX / sizeof(HuffmanCode*)) {
+    return BROTLI_FALSE;


ntrees is less than 1K

eustas · 2026-05-04T08:46:43Z

If any of limits are hard to track, prefer BROTLI_DCHECK to ensure them.

@eustas

…review) Following @eustas review of google#1471: replace early-return overflow guards with BROTLI_DCHECK assertions. This preserves the intent (document invariants) while avoiding any runtime control-flow impact. Per reviewer suggestion: "If any of limits are hard to track, prefer BROTLI_DCHECK to ensure them." - block_splitter_inc.h FN(ClusterBlocks): addition bound check - block_splitter_inc.h FN(SplitByteVector): 2 multiplication bound checks - block_splitter.c CopyLiteralsToByteArray: wrap guard - block_encoder_inc.h FN(BuildAndStoreEntropyCodes): multiplication bound - dec/state.c BrotliDecoderHuffmanTreeGroupInit: 3 bound checks

SongT-50 · 2026-05-04T10:07:47Z

수정된 전체 초안 Hello @eustas, Thank you for the detailed feedback. I completely agree that the guards were dead code and that early returns could cause unexpected behavior if ever reached. I have followed your advice and replaced them with BROTLI_DCHECKs to document these invariants without affecting control flow. I've opened a new, clean PR here: #1472 Please let me know if it looks good to you. Thanks! 2026년 5월 4일 (월) 오후 5:47, Eugene Kliuchnikov ***@***.***>님이 작성:

*eustas* left a comment (google/brotli#1471) <#1471 (comment)> If any of limits are hard to track, prefer BROTLI_DCHECK to ensure them. — Reply to this email directly, view it on GitHub <#1471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BJQZSLCHW2RHSJ4BAVQVLQ34ZBKIVAVCNFSM6AAAAACYO2N3PGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGNRZGQ4DINJQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ᐧ

@eustas

…2nd review) Following @eustas's review of PR v2: > BROTLI_DCHECK declares some readable fact, e.g. "we know/expect that > alphabet size is not more than this constant"; in this PR BROTLI_DCHECK > checks that overflow should not happen - that does not help reader to > understand why. This v3 changes the BROTLI_DCHECKs from overflow-check form (X * Y < SIZE_MAX) to declarative-invariant form (X <= LIMIT), using the bounds @eustas specified inline on PR google#1471. Changes (per @eustas inline review of google#1471) - block_splitter_inc.h SplitByteVector: data_size <= 704 (BROTLI_NUM_COMMAND_SYMBOLS, the largest alphabet that reaches SplitByteVector), num_histograms <= 100 (kMaxLiteralHistograms), bitmaplen <= 13 (derived: (100 + 7) >> 3), length <= 16 MiB (meta-block length cap per RFC 7932). - block_splitter_inc.h ClusterBlocks: num_blocks <= 16 MiB. - block_splitter.c CopyLiteralsToByteArray: BROTLI_DCHECK removed, replaced with an explanatory comment. Per @eustas: "this situation is not possible". Both insert_len and from_pos are bounded by the meta-block length (<= 16 MiB per RFC 7932), so from_pos + insert_len cannot wrap. - block_encoder_inc.h BuildAndStoreEntropyCodes: histograms_size <= 257 (number of block types per RFC 7932), histogram_length_ <= 704 (BROTLI_NUM_COMMAND_SYMBOLS). - dec/state.c BrotliDecoderHuffmanTreeGroupInit: * alphabet_size_limit <= BROTLI_NUM_DISTANCE_SYMBOLS (== 1128 per RFC 7932; the distance alphabet is the largest one handled by the decoder). * max_table_size <= BROTLI_NUM_DISTANCE_SYMBOLS + 376 (== 1504), derived from the alphabet bound plus the 376-entry second-level mix-table overhead documented just above. * ntrees < 1024. BROTLI_MAX_NUMBER_OF_BLOCK_TYPES is 256, so 1024 leaves comfortable margin. A single DCHECK at line 172 covers both the code_size and htree_size multiplications that follow. Constants verified against c/common/constants.h, c/dec/huffman.h, and c/enc/block_splitter.c: * BROTLI_NUM_DISTANCE_SYMBOLS == 1128 (constants.h:70 comment) * BROTLI_NUM_COMMAND_SYMBOLS == 704 (constants.h:25) * BROTLI_MAX_NUMBER_OF_BLOCK_TYPES == 256 (constants.h:21) * HISTOGRAMS_PER_BATCH == 64 (block_splitter.c:86, file-local define) The names BROTLI_MAX_ALPHABET_SIZE, BROTLI_HUFFMAN_MAX_TABLE_SIZE, and BROTLI_HUFFMAN_MAX_TABLE_SIZE_CODE_LENGTHS are not present in the tree; literal numeric bounds with explanatory comments are used in the corresponding slots, deliberately avoiding the HUFFMAN_MAX_CODE_LENGTH-derived value (15) which applies only to the 18-symbol code-lengths tree, not to the literal/distance trees handled here. Build: cmake -DCMAKE_BUILD_TYPE=Debug succeeds with no warnings or errors.

eustas reviewed May 4, 2026

View reviewed changes

eustas closed this May 4, 2026

SongT-50 mentioned this pull request May 4, 2026

enc/dec: add BROTLI_DCHECKs for size computation bounds (per @eustas review) #1472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

enc/dec: harden integer overflow checks in size computations (defense-in-depth)#1471

enc/dec: harden integer overflow checks in size computations (defense-in-depth)#1471
SongT-50 wants to merge 1 commit into
google:masterfrom
SongT-50:harden-integer-overflow-checks

SongT-50 commented May 3, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas May 4, 2026

Uh oh!

eustas commented May 4, 2026

Uh oh!

SongT-50 commented May 4, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

SongT-50 commented May 3, 2026

Summary

Sites covered

Severity / impact (honest disclosure)

Compatibility

Tests

Honest disclosure

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eustas commented May 4, 2026

Uh oh!

SongT-50 commented May 4, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants