Skip to content

Fix zlib headers for preset dictionary compression - #257

Open
glinkinvd wants to merge 1 commit into
pycompression:developfrom
glinkinvd:fix-zlib-dictionary-header
Open

glinkinvd wants to merge 1 commit into
pycompression:developfrom
glinkinvd:fix-zlib-dictionary-header

Conversation

@glinkinvd

Copy link
Copy Markdown

Streams produced by isal_zlib.compressobj(zdict=...) can fail when decoded by Python's standard zlib with the same dictionary: invalid distance too far back. The deflate stream uses dictionary history, but the zlib wrapper omits FDICT and DICTID.

This change writes the dictionary header once before the first compress() or flush(), then lets ISA-L produce deflate data and the Adler-32 trailer. DICTID covers the full supplied dictionary and is stored explicitly in network byte order, including with the older ISA-L revision bundled by this repository. Raw deflate and empty dictionaries keep their existing paths.

Adds 155 regression checks covering levels, window sizes, short and long dictionaries, empty input, flush-first operation, repeated flushes, and cross-decoding with standard zlib. The smallest reproducer uses dictionary = b'hello world' and data = dictionary * 10 at compression level 1.

Validation on Linux x86_64 / CPython 3.14.7:

  • Full suite on this branch: 9598 passed, 6 skipped.
  • Both proposed fixes together, statically linked to the pinned ISA-L revision: 9605 passed, 6 skipped.
  • Both fixes together, dynamically linked to ISA-L 2.32.1: 9605 passed, 6 skipped.
  • Both fixes together under AddressSanitizer and LeakSanitizer: 9605 passed, 6 skipped, exit 0.
  • Full-tree flake8 and mypy passed for the combined tree.

The sanitizer run used CPython 3.14 with PYTHONMALLOC=malloc, detect_leaks=1, and pytest plugin autoload disabled; pytest-timeout was explicitly enabled. It was not the debug-interpreter tox environment. Skips are the existing big-memory and unsupported copy scenarios.

Checklist

  • Pull request details were added to CHANGELOG.rst
  • Documentation was reviewed; no API changes require an update

@rhpvorderman

Copy link
Copy Markdown
Collaborator

Thanks. I will take a look at this next week. Thanks for the extra regression tests!

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.

2 participants