Direct Pivco-Huffman port - #66
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56c4fa9598
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| blk_enc_len = get_u32(p); p += 4; | ||
| if (p + blk_enc_len > body_end) { err = PIVCOHUF_ERR_TOO_SHORT; break; } | ||
| blk_remaining = uncomp_size - written; | ||
| blk_out = (blk_remaining >= B) ? (out + written) : block_buf; |
There was a problem hiding this comment.
When loading corrupted or attacker-controlled serialized bytes, the encoded block carries its own symbol count, but blk_out capacity is selected solely from the independent file-level B and uncompressed-size fields. For example, changing a valid 32,768-symbol stream's header to declare an uncompressed size and block size of 1 makes pivco_decode write the embedded 32,768 symbols into a one-byte vector; this produces an ASan-confirmed heap-buffer-overflow. Parse and validate the block's count against both the destination capacity and declared block size before decoding.
AGENTS.md reference: AGENTS.md:L86-L92
Useful? React with 👍 / 👎.
| list(APPEND PIXIE_PIVCO_DEFINITIONS | ||
| PIVCO_HAS_AVX2=1 | ||
| PIVCO_HAS_SSE4=1) | ||
| list(APPEND PIXIE_PIVCO_OPTIONS | ||
| -mavx2 | ||
| -mbmi2 | ||
| -msse4.1 | ||
| -mpopcnt) |
There was a problem hiding this comment.
On every x86_64/amd64 build, this unconditionally defines the AVX2 backend and compiles all codec and common sources with AVX2, BMI2, SSE4.1, and POPCNT. Since pivco_encode and pivco_decode statically select the x86 worker whenever these definitions exist, binaries run on older x86-64 CPUs—or builds configured with PIXIE_DISABLE_BMI2=ON—can execute unsupported instructions instead of using the included scalar fallback. Detect the required features or honor the fallback options before enabling this tier.
AGENTS.md reference: AGENTS.md:L348-L353
Useful? React with 👍 / 👎.
| /* HEADER_CHECKSUM verification disabled (2026-05-12) -- bytes are | ||
| * still in the format at offset 22..25, currently always zero. */ |
There was a problem hiding this comment.
For any stream damaged in storage or transit, the decoder ignores both checksum fields even though the public pivcohuf_decompress contract says it verifies them and the wire-format documentation specifically relies on the header checksum to protect BODY_LENGTH. The encoder also writes zero checksums, so corruption can silently produce incorrect output or feed corrupted metadata into unsafe length arithmetic rather than returning a checksum error.
AGENTS.md reference: AGENTS.md:L86-L92
Useful? React with 👍 / 👎.
| target_compile_options(${codec_target} | ||
| PRIVATE -O3 ${PIXIE_PIVCO_OPTIONS}) |
There was a problem hiding this comment.
When the repository's asan preset is used, ENABLE_ADDRESS_SANITIZER adds sanitizer flags only to CMAKE_CXX_FLAGS; these newly added .c targets therefore compile without -fsanitize=address and are additionally forced to -O3. As a result, memory errors inside the codec—including out-of-bounds SIMD stores—are not checked by the fallback validation job even though the C++ test executable links ASan. Apply the sanitizer settings to the C targets as well.
AGENTS.md reference: AGENTS.md:L356-L357
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 90.66% 90.67% +0.01%
==========================================
Files 47 50 +3
Lines 12220 12290 +70
Branches 1771 1778 +7
==========================================
+ Hits 11079 11144 +65
- Misses 533 536 +3
- Partials 608 610 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
No description provided.