Skip to content

Various bug fixes - #326

Closed
BrianPugh wants to merge 2 commits into
mainfrom
code-review-bug-fixes
Closed

Various bug fixes#326
BrianPugh wants to merge 2 commits into
mainfrom
code-review-bug-fixes

Conversation

@BrianPugh

Copy link
Copy Markdown
Owner

A comprehensive review (line-by-line C scan, cross-implementation sync audit, API-contract tracing, and fuzzing) confirmed 17 bugs. Every fix below was validated bidirectionally where a test could reach it: the new regression tests fail on the pre-fix code and pass on the fixed code.

C core (tamp/_c_src/tamp/)

  • compressor.c: silent data loss in the extended-format RLE path. A lone run byte consumed into rle_count by one poll was discarded when a later poll saw the run had ended ("AAB" round-tripped as "AB" with byte-at-a-time sink/poll). The byte is now re-emitted as a literal, mirroring the existing rle_count==1 drain in tamp_compressor_flush.

  • compressor.c: silent output corruption with lazy_matching + extended (the configuration shipped in the Python wheels). poll_extended_handling consumed input and mutated the window without invalidating the cached lazy match, so a stale match was later emitted for data that no longer matched. Found by round-trip fuzzing (~5% of run-heavy seeds corrupted). The cache is now cleared whenever extended handling consumes input. The pure-Python reference mirrors both fixes.

  • compressor.c: tamp_compress_stream spun forever when TAMP_STREAM_WORK_BUFFER_SIZE/2 < 6: an extended match token could never fit in the output half-buffer, poll returned TAMP_OUTPUT_FULL with zero progress, and the loop never detected the stall. Now a compile-time #error (buffer must be >= 12 when extended compression is compiled in).

  • decompressor.c: with TAMP_EXTENDED_DECOMPRESS=0, extended streams were rejected only after committing conf and setting configured=true, so a retrying caller silently decoded the extended stream as classic format. Validation now happens before any state is committed.

  • compressor_find_match_desktop.c: input_word_ext was unconditionally assembled from input_bytes[2..9] even when only input_bytes[0..max-1] were initialized (uninitialized stack read, UB, MSan finding). The word is now only built when max_pattern_size >= 10.

Python bindings (Cython + pure Python)

  • _c_decompressor.pyx: a user-supplied dictionary was passed to tamp_decompressor_init with no length validation; a too-small buffer let C write past the end of the bytearray (reproduced as a segfault). Now raises ValueError on size mismatch, matching the compressor. decompressor.py gets the same validation.

  • decompressor.py: a dictionary supplied for a stream whose header does not set the custom-dictionary bit was used verbatim as the window, silently corrupting all output (C/Cython re-initialize it). The buffer is now re-initialized in place to match the C implementation.

  • _c_decompressor.pyx + decompressor.py: read(size) with a positive size returned size bytes zero-padded past end-of-stream instead of a short read (compress(b"hello") then read(100) returned 100 bytes).

  • _c_decompressor.pyx: init accepted any object with read() but later required readinto(), raising AttributeError mid-stream for read()-only file-likes. Falls back to read() when readinto is absent.

  • _c_compressor.pyx: write(b"") raised IndexError (&data[0] on an empty buffer). Now returns 0 like the pure-Python implementation.

MicroPython (viper + native module)

  • decompressor_viper.py: match copies read w_buf_ptr[index + i] with no bound on index + match_size, so a corrupt/malicious stream leaked up to ~14 bytes of adjacent heap into the output via unchecked ptr8. Now raises ValueError, matching the C implementation's TAMP_OOB. Also validates custom-dictionary size (unchecked ptr8 writes had the same out-of-bounds exposure).

  • compressor_viper.py: the match-extension loop read window_buf[window_index + k] before evaluating the bounds check in the same or-expression - a 1-byte out-of-bounds ptr8 read at the window end. The bounds check now short-circuits first.

  • mpy_bindings/bindings.c: decompressor_make_new passed the input buffer capacity (CHUNK_SIZE) instead of the actual bytes read to tamp_decompressor_read_header, parsing uninitialized m_malloc memory as the header on short/empty streams; a truncated header (positive TAMP_INPUT_EXHAUSTED, not caught by TAMP_CHECK) also slipped through. Both now raise ValueError.

  • mpy_bindings/bindings.c: initialize_dictionary took (buffer, literal) positionally while CPython's signature is (source, seed=None, literal=8), so the same call built different dictionaries on device vs desktop (cross-platform corruption). The signature now matches CPython; unsupported custom seeds raise ValueError instead of silently diverging.

WebAssembly (wasm/src/)

  • tamp.js: flush() defaulted write_token=false while every Python implementation defaults True, so a JS user following the Python mid-stream-flush pattern produced a desynchronized bitstream (padding bits decoded as token bits; most payloads corrupt). The default is now true; the compression stream's end-of-stream flush passes an explicit false; tamp.d.ts documents the semantics.

CLI / build

  • cli/build_dictionary.py: the module unconditionally imported the Cython-only _c_build_dictionary, so the entire tamp CLI crashed with ImportError on installs without C extensions - even tamp compress --implementation python. The import is now guarded; only the build-dictionary command itself requires the compiled kernels and raises a clear RuntimeError.

  • build.py: TAMP_SANITIZE=1 on Windows crashed with NameError (extra_link_args unbound on that branch). Sanitizers were never actually applied there; now raises a clear "not supported on Windows".

Tests

  • ctests: new test_compressor_extended_rle_lone_byte_sink_poll (fails pre-fix: "Expected 3 Was 2") and test_compressor_extended_lazy_rle_fuzz (1000 deterministic seeds; pre-fix corrupts ~5% of them).

  • ctests now build with -DTAMP_LAZY_MATCHING=1. The lazy-matching configuration shipped in every Python wheel was previously untested by make c-test, which is how the lazy-cache corruption survived.

  • Makefile: fixed the c-test-embedded link rule (missing LittleFS/FatFS objects; the target did not link from a clean build). Both variants now pass 31/31.

  • tests/test_bug_regressions.py: 7 tests covering the Python-level fixes across the pure-Python and Cython implementations. On pre-fix code they fail in exactly the implementations that had each bug; the dictionary-size test is excluded from that comparison because pre-fix it segfaults the interpreter.

  • wasm/test/basic-test.js: mid-stream flush round-trip with default arguments; payload chosen to provably desync on the pre-fix default (byte-aligned payloads can mask the bug).

Verified: pytest 136 passed (+247 subtests); make c-test and c-test-embedded 31/31; wasm 84/84; make mpy links (EM_ARM); ruff clean.

Compatibility notes

No bitstream format changes; new streams decode on old decompressors and vice versa. Compressed output bytes can differ from prior releases for run-heavy inputs (the previously-corrupt paths). Visible behavior changes, each replacing incorrect or dangerous behavior:

  1. WASM flush() default changed false -> true; a bare final flush() now appends a FLUSH token (1-2 bytes, decodes identically).
  2. MicroPython native initialize_dictionary(buf, literal) positional form now raises; use initialize_dictionary(buf, None, literal).
  3. Decompressor dictionaries must be exactly 2**window bytes (the compressor already enforced this).
  4. Building with TAMP_STREAM_WORK_BUFFER_SIZE in [4, 11] and extended compression enabled is now a compile error instead of a runtime hang.

A comprehensive review (line-by-line C scan, cross-implementation sync
audit, API-contract tracing, and fuzzing) confirmed 17 bugs. Every fix
below was validated bidirectionally where a test could reach it: the new
regression tests fail on the pre-fix code and pass on the fixed code.

C core (tamp/_c_src/tamp/)
--------------------------

* compressor.c: silent data loss in the extended-format RLE path. A lone
  run byte consumed into rle_count by one poll was discarded when a later
  poll saw the run had ended ("AAB" round-tripped as "AB" with
  byte-at-a-time sink/poll). The byte is now re-emitted as a literal,
  mirroring the existing rle_count==1 drain in tamp_compressor_flush.

* compressor.c: silent output corruption with lazy_matching + extended
  (the configuration shipped in the Python wheels). poll_extended_handling
  consumed input and mutated the window without invalidating the cached
  lazy match, so a stale match was later emitted for data that no longer
  matched. Found by round-trip fuzzing (~5% of run-heavy seeds corrupted).
  The cache is now cleared whenever extended handling consumes input.
  The pure-Python reference mirrors both fixes.

* compressor.c: tamp_compress_stream spun forever when
  TAMP_STREAM_WORK_BUFFER_SIZE/2 < 6: an extended match token could never
  fit in the output half-buffer, poll returned TAMP_OUTPUT_FULL with zero
  progress, and the loop never detected the stall. Now a compile-time
  #error (buffer must be >= 12 when extended compression is compiled in).

* decompressor.c: with TAMP_EXTENDED_DECOMPRESS=0, extended streams were
  rejected only after committing conf and setting configured=true, so a
  retrying caller silently decoded the extended stream as classic format.
  Validation now happens before any state is committed.

* compressor_find_match_desktop.c: input_word_ext was unconditionally
  assembled from input_bytes[2..9] even when only input_bytes[0..max-1]
  were initialized (uninitialized stack read, UB, MSan finding). The word
  is now only built when max_pattern_size >= 10.

Python bindings (Cython + pure Python)
--------------------------------------

* _c_decompressor.pyx: a user-supplied dictionary was passed to
  tamp_decompressor_init with no length validation; a too-small buffer
  let C write past the end of the bytearray (reproduced as a segfault).
  Now raises ValueError on size mismatch, matching the compressor.
  decompressor.py gets the same validation.

* decompressor.py: a dictionary supplied for a stream whose header does
  not set the custom-dictionary bit was used verbatim as the window,
  silently corrupting all output (C/Cython re-initialize it). The buffer
  is now re-initialized in place to match the C implementation.

* _c_decompressor.pyx + decompressor.py: read(size) with a positive size
  returned `size` bytes zero-padded past end-of-stream instead of a short
  read (compress(b"hello") then read(100) returned 100 bytes).

* _c_decompressor.pyx: __init__ accepted any object with read() but
  later required readinto(), raising AttributeError mid-stream for
  read()-only file-likes. Falls back to read() when readinto is absent.

* _c_compressor.pyx: write(b"") raised IndexError (&data[0] on an empty
  buffer). Now returns 0 like the pure-Python implementation.

MicroPython (viper + native module)
-----------------------------------

* decompressor_viper.py: match copies read w_buf_ptr[index + i] with no
  bound on index + match_size, so a corrupt/malicious stream leaked up to
  ~14 bytes of adjacent heap into the output via unchecked ptr8. Now
  raises ValueError, matching the C implementation's TAMP_OOB. Also
  validates custom-dictionary size (unchecked ptr8 writes had the same
  out-of-bounds exposure).

* compressor_viper.py: the match-extension loop read
  window_buf[window_index + k] before evaluating the bounds check in the
  same or-expression - a 1-byte out-of-bounds ptr8 read at the window
  end. The bounds check now short-circuits first.

* mpy_bindings/bindings.c: decompressor_make_new passed the input buffer
  capacity (CHUNK_SIZE) instead of the actual bytes read to
  tamp_decompressor_read_header, parsing uninitialized m_malloc memory as
  the header on short/empty streams; a truncated header (positive
  TAMP_INPUT_EXHAUSTED, not caught by TAMP_CHECK) also slipped through.
  Both now raise ValueError.

* mpy_bindings/bindings.c: initialize_dictionary took (buffer, literal)
  positionally while CPython's signature is (source, seed=None,
  literal=8), so the same call built different dictionaries on device vs
  desktop (cross-platform corruption). The signature now matches CPython;
  unsupported custom seeds raise ValueError instead of silently
  diverging.

WebAssembly (wasm/src/)
-----------------------

* tamp.js: flush() defaulted write_token=false while every Python
  implementation defaults True, so a JS user following the Python
  mid-stream-flush pattern produced a desynchronized bitstream (padding
  bits decoded as token bits; most payloads corrupt). The default is now
  true; the compression stream's end-of-stream flush passes an explicit
  false; tamp.d.ts documents the semantics.

CLI / build
-----------

* cli/build_dictionary.py: the module unconditionally imported the
  Cython-only _c_build_dictionary, so the entire `tamp` CLI crashed with
  ImportError on installs without C extensions - even
  `tamp compress --implementation python`. The import is now guarded;
  only the build-dictionary command itself requires the compiled kernels
  and raises a clear RuntimeError.

* build.py: TAMP_SANITIZE=1 on Windows crashed with NameError
  (extra_link_args unbound on that branch). Sanitizers were never
  actually applied there; now raises a clear "not supported on Windows".

Tests
-----

* ctests: new test_compressor_extended_rle_lone_byte_sink_poll (fails
  pre-fix: "Expected 3 Was 2") and test_compressor_extended_lazy_rle_fuzz
  (1000 deterministic seeds; pre-fix corrupts ~5% of them).

* ctests now build with -DTAMP_LAZY_MATCHING=1. The lazy-matching
  configuration shipped in every Python wheel was previously untested by
  make c-test, which is how the lazy-cache corruption survived.

* Makefile: fixed the c-test-embedded link rule (missing LittleFS/FatFS
  objects; the target did not link from a clean build). Both variants now
  pass 31/31.

* tests/test_bug_regressions.py: 7 tests covering the Python-level fixes
  across the pure-Python and Cython implementations. On pre-fix code they
  fail in exactly the implementations that had each bug; the
  dictionary-size test is excluded from that comparison because pre-fix
  it segfaults the interpreter.

* wasm/test/basic-test.js: mid-stream flush round-trip with default
  arguments; payload chosen to provably desync on the pre-fix default
  (byte-aligned payloads can mask the bug).

Verified: pytest 136 passed (+247 subtests); make c-test and
c-test-embedded 31/31; wasm 84/84; make mpy links (EM_ARM); ruff clean.

Compatibility notes
-------------------

No bitstream format changes; new streams decode on old decompressors and
vice versa. Compressed output bytes can differ from prior releases for
run-heavy inputs (the previously-corrupt paths). Visible behavior
changes, each replacing incorrect or dangerous behavior:

1. WASM flush() default changed false -> true; a bare final flush() now
   appends a FLUSH token (1-2 bytes, decodes identically).
2. MicroPython native initialize_dictionary(buf, literal) positional form
   now raises; use initialize_dictionary(buf, None, literal).
3. Decompressor dictionaries must be exactly 2**window bytes (the
   compressor already enforced this).
4. Building with TAMP_STREAM_WORK_BUFFER_SIZE in [4, 11] and extended
   compression enabled is now a compile error instead of a runtime hang.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a set of correctness and memory-safety bugs across tamp’s C core, Python/Cython bindings, MicroPython bindings, and the WASM implementation, and adds targeted regression tests to prevent reintroduction.

Changes:

  • Fixes multiple compressor/decompressor edge cases (RLE lone-byte loss, lazy-match cache invalidation, extended-stream validation ordering, stream buffer deadlock, and UB in desktop match finder).
  • Hardens dictionary handling and stream I/O behavior across Python/Cython/MicroPython/WASM (size validation, read(size) behavior, readinto fallback, and WASM flush default semantics).
  • Expands test coverage with new C, Python, and WASM regression tests and adjusts build/test wiring.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
wasm/test/basic-test.js Adds WASM regression test for mid-stream flush default round-trip.
wasm/src/tamp.js Changes flush() default to write a FLUSH token and documents correct usage.
wasm/src/tamp.d.ts Updates TypeScript docs to reflect new flush default and semantics.
wasm/src/streams.js Ensures end-of-stream flush explicitly uses flush(false) to avoid extra token.
tests/test_bug_regressions.py Adds Python-level regression tests covering the fixed bugs across implementations.
tamp/decompressor.py Adds dictionary length validation, reinitializes unused dictionaries, and fixes read(size) past-EOF behavior.
tamp/decompressor_viper.py Adds dictionary size validation and OOB match-reference checks for MicroPython viper decompressor.
tamp/compressor.py Fixes extended-format RLE lone-byte handling and invalidates lazy-match cache when extended paths consume input.
tamp/compressor_viper.py Fixes OOB read risk by reordering bounds checks in MicroPython viper compressor.
tamp/cli/build_dictionary.py Guards C-extension imports so CLI can load without compiled modules; errors clearly when build-dictionary is invoked.
tamp/_c_src/tamp/decompressor.c Rejects extended streams before committing decompressor state when extended support is disabled.
tamp/_c_src/tamp/compressor.c Fixes extended RLE lone-byte emission, clears lazy-match cache on extended handling, and adds compile-time stream buffer sizing guard.
tamp/_c_src/tamp/compressor_find_match_desktop.c Avoids uninitialized stack reads by only assembling input_word_ext when enough input bytes exist.
tamp/_c_decompressor.pyx Adds dictionary-size validation, readinto/read fixes, and readinto fallback behavior for read()-only streams.
tamp/_c_compressor.pyx Fixes empty-write handling to avoid &data[0] on empty buffers.
mpy_bindings/bindings.c Fixes MicroPython header parsing length bug, improves API signature compatibility for initialize_dictionary, and tightens error handling.
Makefile Builds ctests with -DTAMP_LAZY_MATCHING=1 and fixes embedded link rule inputs.
ctests/test_runner.c Registers new compressor regression tests.
ctests/test_compressor.c Adds C regression tests for extended RLE lone-byte loss and lazy+extended fuzz corruption.
build.py Provides a clear error when sanitizers are requested on Windows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tamp/_c_decompressor.pyx Outdated
Comment on lines 69 to 75
if dictionary and bit_size(len(dictionary) - 1) != conf.window:
# A wrong-size buffer used as the window would let the C decompressor
# write out of bounds.
raise ValueError("Dictionary-window size mismatch.")

self._window_buffer = dictionary if dictionary else bytearray(1 << conf.window)
self._window_buffer_ptr = <unsigned char *>self._window_buffer

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fa44b6f: validation now requires the exact size (len(dictionary) != (1 << conf.window)) and uses is not None, so empty buffers are rejected instead of bypassing. The same insufficient bit_size check pre-existed in both compressors and is fixed there too. The regression test now sweeps wrong sizes {256, 2560, 0} — 2560 covers the range the old check wrongly accepted.

Comment thread tamp/decompressor.py Outdated
Comment thread mpy_bindings/bindings.c Outdated
Comment thread mpy_bindings/bindings.c
CI fixes:

* compressor_find_match_desktop.c: zero-initialize input_bytes instead of
  conditionally building input_word_ext. The previous approach tripped
  GCC's -Werror=maybe-uninitialized on the manylinux and MicroPython
  builds (GCC cannot prove the fill loop covers indices 0-1). Full
  initialization resolves both that error and the original MSan
  uninitialized-read finding, at the cost of one 16-byte zero store in
  the desktop-only implementation.

* wasm one-shot compress(): pass an explicit boolean to the final
  flush(). compressionOptions.dictionary_reset is undefined when the
  caller does not specify it, which silently picked up flush()'s new
  mid-stream default (write_token=true) and appended a FLUSH token to
  all one-shot output - the cause of the enwik8 ground-truth hash
  mismatch. One-shot output is again byte-identical to the pinned hash
  (verified locally: CLI and JS enwik8 both produce d9d804c9...).

Copilot review feedback:

* Dictionary size validation now requires the exact window size
  (len == 1 << window). The previous bit_size(len - 1) == window check
  (pre-existing in the compressors, copied into the decompressor fix)
  accepted any length in (2**(window-1), 2**window], e.g. a 2560-byte
  buffer for window=12, which still allowed out-of-bounds window writes.
  Applied to _c_compressor.pyx, _c_decompressor.pyx, compressor.py, and
  decompressor.py.

* Dictionary presence is now tested with `is not None` instead of
  truthiness in all four files, so an empty bytearray is validated (and
  rejected) rather than silently treated as "no dictionary provided".

* mpy_bindings/bindings.c: clearer error messages for the unsupported
  initialize_dictionary seed ("custom seed unsupported") and truncated/
  invalid headers ("invalid header").

* tests/test_bug_regressions.py: the dictionary-size test now sweeps
  wrong sizes {256, 2560, 0} - 2560 exercises the range the old
  bit_size check wrongly accepted, 0 exercises the truthiness bypass -
  against both the compressor and decompressor of both implementations.

Verified: pytest 136 passed (+253 subtests); c-test and c-test-embedded
31/31; wasm 84/84; make mpy links; CLI and JS enwik8 hashes match the
pinned ground truth; ruff clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@BenBE

BenBE commented Jul 5, 2026

Copy link
Copy Markdown

Fixing such a large number of issues across the code base in one single commit is a nightmare to review. Each commit should ideally only touch a single section of the code and be focused on one particular topic. Even while splitting every bug fix (+ adding a testcase for it) into its own commit will end up with over a dozen commits, this helps any reviewer of this PR because related changes are concentrated and the overall changes per commit remain minimal. This particularly helps when – like is the case here – additional modifications and bug fixes have to be made because the original set of changes was incomplete or erroneous. Please take a look at git rebase to split you set of changes into a more manageable size by splitting these two large commits into individual commits for the various bug fixes. Given this set of changes covers multiple components of the implementation, splitting by these boundaries (one commit per binding) is also a viable approach to cutting these changes into commits.

@BrianPugh

Copy link
Copy Markdown
Owner Author

yup! don't worry i'm going to have Claude break it up; was just pushing this while Fable was publicly available incase i didn't get around to it by July 7th 😛 .

BrianPugh added a commit that referenced this pull request Jul 6, 2026
Numbers reflect the tree with the six bug-fix branches (the #326 split)
merged; merge this PR last.

Runtime table: the MicroPython native module row re-measured on a Pi Pico
(RP2040, 125 MHz) running MicroPython v1.26.1 firmware, with the C-core
fixes compiled in: 29,997 B/s compression, 980,392 B/s decompression
(was 31,328 / 990,099; within measurement noise of unmodified main).
The surrounding prose now states the firmware version. The C and deflate
rows were not re-measured and are unchanged.

Binary Size table: regenerated with `make binary-size` (arm-none-eabi-gcc
15.2.1, MicroPython v1.27) on the merged tree: extended compressor builds
grew 58 bytes (RLE lone-byte fix), no-extended decompressor builds shrank
22 bytes (early extended rejection), and the MicroPython Native row is
now 5321/4765/8993 (bindings fixes + C-core fixes). Heatshrink and uzlib
rows unchanged (not re-measured).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013UMgpFKUcWjqCDKcZwiPog
@BrianPugh BrianPugh closed this Jul 6, 2026
@BrianPugh
BrianPugh deleted the code-review-bug-fixes branch July 7, 2026 19:39
@BrianPugh

Copy link
Copy Markdown
Owner Author

by the way @BenBE, if you want to give main a try, i've merged in all the changes I think i want to make for v2.3.0. I might remove the esp32-optimizations depending on the outcome of #338. Below is the draft changelog.


This is a correctness- and robustness-focused release. The compressed bitstream format is unchanged and remains backwards-compatible; all fixes below preserve round-trip compatibility with streams produced by earlier 2.x versions.

Bug Fixes

Optimizations

Other

Full Changelog: v2.2.4...v2.3.0

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.

3 participants