Skip to content

Fix empty Blosc buffer round-trips - #854

Open
fallenmi wants to merge 3 commits into
zarr-developers:mainfrom
fallenmi:agent/fix-blosc-empty-roundtrip
Open

Fix empty Blosc buffer round-trips#854
fallenmi wants to merge 3 commits into
zarr-developers:mainfrom
fallenmi:agent/fix-blosc-empty-roundtrip

Conversation

@fallenmi

@fallenmi fallenmi commented Aug 16, 2026

Copy link
Copy Markdown

Fixes #831.

Summary

  • validate the declared Blosc frame before decompression without rejecting trailing bytes
  • treat a validated zero-length frame as a successful decompression
  • preserve the previous error for an empty frame paired with a non-empty destination
  • require the decompressor to return the full declared uncompressed size
  • require system Blosc 1.16.0+, where blosc_cbuffer_validate was introduced
  • add regression coverage and an unreleased fix note

The validation first proves that the complete 16-byte header is present, then bounds the
declared frame size by the actual input and validates exactly that frame. This rejects
truncation while preserving the existing behavior of ignoring bytes after the first frame.

Validation

  • pre-review fix: focused Blosc regression/error tests passed (15 passed, 1 expected skip)
  • pre-review fix: full local suite passed (650 passed, 32 optional-dependency skips)
  • pre-review adversarial empty/non-empty, buffer-type, compressor, shuffle, threading, and
    malformed-frame matrix: 6,193 checks, 0 errors
  • current review follow-up: Cython 3.1.3 transpilation passed
  • current review follow-up: independent static/adversarial review and git diff --check passed
  • current review follow-up runtime tests: pending maintainer approval of fork workflows

Checklist

  • Unit tests and/or doctests in docstrings
  • Tests pass locally on the current head
  • No new or modified user-facing classes or functions
  • Changes documented in docs/release.md
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

AI assistance

OpenAI Codex assisted with reproduction, implementation, test planning, validation, review,
and the follow-up changes. The account owner authorized publication; the broad fork workflows
require maintainer approval before they can run.

@fallenmi
fallenmi marked this pull request as ready for review August 25, 2026 12:41
@d-v-b

d-v-b commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Thanks for the fix — the diagnosis of #831 is right, and the mechanical concerns all check out (no buffer leaks on the early return, validation is O(1), the vendored c-blosc 1.21.7 provides blosc_cbuffer_validate). A few findings, ranked by severity:

Correctness / compat

  1. Frames with trailing bytes now fail to decompress (blosc.pyx#L364). blosc_cbuffer_validate requires the passed length to exactly equal the frame's declared cbytes (c-blosc does if (header_cbytes != cbytes) return -1;), whereas the old blosc_cbuffer_sizes path ignored the Python-level buffer length. So decompress(compress(data) + b'\x00') — or any padded/over-read buffer (fixed-size block reads, mmap slices, chunk files with stale trailing bytes after a shorter in-place overwrite) — previously round-tripped and now raises. Blosc.decode forwards the buffer untrimmed, so this reaches stored-data reads directly. The PR description calls this out as intentional, but the release note ("size-mismatched") undersells that previously-valid input now fails, and the decompress docstring wasn't updated. If strictness is only wanted against truncation, header_cbytes <= cbytes semantics (manual check via blosc_cbuffer_sizes after a min-length check) would reject truncation without breaking padded buffers.

  2. Empty frame + larger caller-provided out now silently returns the untouched buffer (blosc.pyx#L390). Previously, decoding a valid empty frame into a non-empty out (e.g. chunk metadata promises 1000 bytes but the store holds an empty frame) raised RuntimeError; now the dest_nbytes < nbytes guard passes trivially and the caller gets uninitialized data with no error. This is arguably consistent with how oversized out works for non-empty frames, but it converts a formerly loud corruption signal into silent garbage — worth an explicit decision, and a test either way.

  3. The root cause is the ret <= 0 check, and it's still there (blosc.pyx#L406). As the new comment itself notes, blosc_decompress returns the decompressed byte count, so 0 is success for an empty frame — the bug is that ret <= 0 misclassifies it. Since nbytes is now validated and trusted, if ret < 0 or <size_t>ret != nbytes handles the empty case with no special branch and catches partial decompression (today 0 < ret < nbytes returns a partially-filled buffer as success). That would let the nbytes == 0 early return be dropped entirely.

Consistency / diagnostics

  1. Validation is applied only in decompress_cbuffer_sizes (blosc.pyx#L148), the public cbuffer_complib, and _cbuffer_metainfo still read a 16-byte header from unvalidated buffers, so e.g. cbuffer_complib(b'x') still reads out of bounds — the same malformed-input class this PR targets.

  2. The validation error message is misleading (blosc.pyx#L366): blosc_cbuffer_validate only ever returns -1, so this always reads error during blosc decompression: -1 — byte-identical to the genuine decompression failure at L407, even though no decompression was attempted. A distinct message (e.g. invalid blosc frame (header validation failed), in the style of L321's compression error) would make truncated/padded/corrupt inputs diagnosable.

Tests / docs

  1. The two behavior changes above have no regression coverage: in test_empty_encode_decode every out buffer is 0-length, so the early return is only exercised with dest_nbytes == 0. decompress(empty_frame, out=bytearray(10)) and decompress(enc + b'pad') are the actual new edges and neither is pinned by a test.

  2. Empty-array round-trip is a codec-wide invariant but the coverage is a blosc-only one-off with default Blosc(). Appending np.empty(0, dtype='u1') to the module's arrays list would exercise it across all 12 parametrized configurations (shuffle/cname variants are exactly where an empty-frame regression could differ), and no other codec test module covers empty arrays at all.

  3. The blosc >= 1.16.0 floor isn't in the build docs: docs/index.md's -Dsystem_blosc=enabled instructions now fail on distros shipping older blosc (e.g. 1.14) with no documented explanation — only docs/release.md mentions the floor. Minor related note: with -Dsystem_blosc=auto, an old system blosc now silently falls back to the vendored copy.

Minor: the new test's try/finally restore of blosc.use_threads diverges from the bare-assignment idiom the five sibling tests using the same fixture follow — if restoring the global matters, a yield fixture would give all consumers cleanup for free.

Preserve trailing-byte compatibility, keep empty-frame output failures loud, verify the full decompressed size, and expand regression and build-documentation coverage.

Generated-by: OpenAI Codex
@fallenmi

Copy link
Copy Markdown
Author

Thanks for the detailed review. I addressed the in-scope findings in e3692a4:

  • trailing bytes remain supported by reading a complete header, bounding the declared cbytes
    by the actual buffer, and validating exactly that declared frame;
  • an empty frame paired with a non-empty out remains a loud RuntimeError instead of
    returning an untouched destination;
  • the empty-frame early return is gone, and decompression now succeeds only when the returned
    byte count exactly matches the validated nbytes;
  • header-validation and truncation errors now have distinct diagnostics;
  • empty arrays now run through the full codec matrix, with focused regressions for non-empty
    output, trailing bytes, and truncation;
  • the system-Blosc minimum is now documented in the build instructions.

I left _cbuffer_sizes, cbuffer_complib, and _cbuffer_metainfo unchanged because their
input-validation contract predates #831 and needs a separate API-wide scope and regression
matrix rather than being folded into this empty-roundtrip fix.

The follow-up passes Cython 3.1.3 transpilation and git diff --check; runtime workflows on the
new head are pending maintainer approval.

Append the empty-array case so existing fixture indices keep their previous meaning.

Generated-by: OpenAI Codex
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.

Can't roundtrip compression/decompression with empty numpy array

2 participants