Skip to content

fix: harden encoder/decoder against UB, data races, and corrupted input#57

Open
facontidavide wants to merge 1 commit intomainfrom
fix/hardening-robustness-review
Open

fix: harden encoder/decoder against UB, data races, and corrupted input#57
facontidavide wants to merge 1 commit intomainfrom
fix/hardening-robustness-review

Conversation

@facontidavide
Copy link
Owner

Summary

  • Fix decodeVarint UB: move shift overflow check before the shift operation to prevent undefined behavior on malformed 11+ byte varints
  • Fix decodeVarint underflow: guard against uval==0 (corrupted data) wrapping uint64_t to UINT64_MAX
  • Fix worker thread deadlock: detect dead compression worker on subsequent encode() calls instead of hanging forever
  • Fix WASM encode performance: compute MaxCompressedSize first and encode directly into caller's buffer when it fits, avoiding unnecessary heap allocation + memcpy
  • Clean up worker error semantics: only set worker_failed_ (not compression_done_) in catch block for clearer signal separation
  • Optimize encode(BufferView&): use header_.size() directly instead of re-serializing YAML via MaxCompressedSize
  • Improve error messages: include field name and type in MaxSerializedFieldSize errors
  • Add <cstring> include: make memcpy dependency explicit in encoding_utils.hpp
  • Fix FloatLossy test: kResolution was double, causing CTAD to deduce <double> template on float data — changed to float
  • Re-enable tests: uncomment test_field_encoders.cpp, test_intrinsics.cpp, test_header.cpp in CMakeLists and remove skip-tests from CI

Test plan

  • All 9 unit tests pass (including previously disabled ones)
  • Clean build with no warnings
  • Pre-commit hooks pass (clang-format, yaml, etc.)
  • Verify CI passes on Humble and Jazzy workflows

🤖 Generated with Claude Code

Address issues found during code review:
- Fix decodeVarint shift UB: check overflow before shift operation
- Fix decodeVarint underflow: guard against uval==0 on corrupted data
- Fix worker thread deadlock: detect dead worker on subsequent encode()
- Fix WASM encode: avoid extra allocation by encoding directly when buffer fits
- Clean up waitForCompressionComplete: only set worker_failed_ in error path
- Optimize encode(BufferView&): use header_.size() instead of YAML reserialization
- Improve readability and error messages in DecodeHeader/MaxSerializedFieldSize
- Add explicit <cstring> include in encoding_utils.hpp
- Fix pre-existing FloatLossy test bug: use float instead of double for kResolution
- Re-enable previously commented-out tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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

Comments