Skip to content

Harden Cython bindings against buffer and lifetime misuse - #387

Draft
BrianPugh wants to merge 1 commit into
mainfrom
harden-cython-bindings
Draft

BrianPugh wants to merge 1 commit into
mainfrom
harden-cython-bindings

Conversation

@BrianPugh

Copy link
Copy Markdown
Owner

The Cython bindings hand C raw pointers into Python-owned buffers, so ordinary Python code could invalidate them or feed C bad sizes. None of this is reachable from compressed data alone; it needs a caller or file-like object doing something unusual. Most cases crash the interpreter (segfault / bus error) rather than raising.

  • Dictionary/input buffer resize → use-after-free: <unsigned char*>bytearray takes no buffer export, so e.g. dictionary.clear() or .extend() after construction freed/moved storage C kept using. Both classes now hold a memoryview on the window and internal input buffer for the object's lifetime, so a resize raises BufferError.
  • f.readinto() trusted: an over-reported count made C read past the input buffer, and readinto received the internal bytearray itself, so the common buf[:] = chunk idiom could shrink it. The decompressor now passes a memoryview and rejects None, negative, or too-large counts.
  • Output buffer resized during readinto(): the output pointer was cached across f.readinto(), which can run arbitrary code. It's now re-derived and clamped each iteration.
  • Concurrent use: the C calls run with the GIL released, so 2 threads on one object (or __init__ from another thread) crashed. Concurrent use now raises RuntimeError (zlib/bz2/lzma guard the same case).
  • Use before __init__: Compressor.__new__(Compressor).write(...) ran C on an uninitialized struct; this now raises RuntimeError.
  • window/literal range: values were truncated into 4-bit TampConf bitfields (window=24 became 8 after allocating a 16 MiB window), so they're now validated before use.

Performance

Measured on 2 MB of enwik8: compression, decompression via read(), and 64-byte readinto() calls are all unchanged. A first version took readinto(buf) as a typed memoryview and cost ~15% on small calls, so the signature stays bytearray.

Tests

TestCythonBufferLifetimes in tests/test_bug_regressions.py (7 tests). Each fails or crashes on main except the output-resize one, which only crashes under guard malloc.

🤖 Generated with Claude Code

The bindings hand raw pointers into Python-owned buffers to C, so Python
code could invalidate them or feed C bad sizes:

- Hold a memoryview export on the window/dictionary bytearray and the
  internal input buffer for the object's lifetime, so resizing them
  raises BufferError instead of leaving C with a dangling pointer.
- Pass f.readinto() a memoryview (it can't resize the input buffer) and
  reject a byte count that is None, negative, or larger than the buffer.
- readinto() re-derives its output pointer after each f.readinto() call,
  since that may run code that resizes the caller's buffer.
- Raise RuntimeError when a Compressor/Decompressor is used from 2
  threads at once (the C calls run with the GIL released), or before
  __init__ ran.
- Validate window (8-15) and literal (5-8) before they're truncated
  into 4-bit TampConf bitfields.

None of these are reachable from compressed data alone. Throughput is
unchanged (2 MB enwik8 compress/decompress, and 64-byte readinto calls).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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