Skip to content

Fix sbuf bounds contract#511

Merged
simsong merged 1 commit into
mainfrom
codex/fix-sbuf-contract
Jul 20, 2026
Merged

Fix sbuf bounds contract#511
simsong merged 1 commit into
mainfrom
codex/fix-sbuf-contract

Conversation

@simsong

@simsong simsong commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Summary

  • centralize overflow-safe sbuf_t range validation for comparisons, integer readers, slices, and structure access
  • compare byte zero in memcmp, define zero-length comparisons, and reject one-past-end writes
  • correct slice primary-page arithmetic and prevent out-of-range constructors from forming invalid pointers
  • cast bytes before integer shifts and add substantive boundary coverage for exact-end, one-past-end, SIZE_MAX, margin, and zero-length cases
  • reconcile the audit with GitHub issues and mark the completed sbuf_t checklist items in src/TECH_DEBT.md

Root cause

The safety checks were duplicated and mostly expressed as addition-based bounds tests. Those tests could wrap, while the slice page calculation used the wrong inequality. The comparison implementation also advanced both pointers and subtracted one from the requested length, skipping the first byte and underflowing at zero.

Validation

  • make -j4 check — 3/3 registered programs pass (test_be, test_be20_api, test_dfxml)
  • ASan+UBSan rebuild followed by make -j4 check TESTS=test_be20_api — 1/1 passes
  • git diff --check
  • Jean E01 integration regression, first 1 GiB, default scanner set (XOR excluded), target jean@m57.biz:
    • pristine main and this PR were built with the same Homebrew libewf and ran against the same E01 input.
    • python/bulk_diff.py --features found no differences in any reported feature file, including email.txt.

Closes #502
Refs #497

PR drafted and commit authored by Codex AI Assistant.

@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.63%. Comparing base (db71cd9) to head (5f38142).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   72.42%   72.63%   +0.21%     
==========================================
  Files         113      113              
  Lines       12359    12423      +64     
==========================================
+ Hits         8951     9023      +72     
+ Misses       3408     3400       -8     
Flag Coverage Δ
unittests 72.63% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@simsong
simsong marked this pull request as ready for review July 20, 2026 03:36
Copilot AI review requested due to automatic review settings July 20, 2026 03:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores and hardens the sbuf_t bounds-checking contract (the trust boundary used across scanners) by centralizing overflow-safe range validation and fixing several correctness bugs in comparisons, slicing, integer readers, and writable-buffer access. It also adds targeted boundary tests and updates the audit checklist in TECH_DEBT.md to reflect completed work.

Changes:

  • Centralized overflow-safe bounds validation via has_range() / require_range() and applied it to comparisons, integer readers, slices, and struct accessors.
  • Fixed memcmp semantics (compare byte 0, define zero-length comparisons), corrected slice primary-page arithmetic, and rejected one-past-end writes.
  • Added substantive boundary-focused tests in test_be20_api.cpp and marked the corresponding remediation checklist items as completed.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/TECH_DEBT.md Reconciles open issues and marks the sbuf_t contract checklist items as completed.
src/be20_api/test_be20_api.cpp Adds boundary-condition tests covering memcmp, integer readers, slices/pagesize, constructors, and wbuf.
src/be20_api/sbuf.h Introduces centralized range helpers and fixes memcmp and integer assembly semantics to avoid underflow/UB.
src/be20_api/sbuf.cpp Fixes slice/pagesize calculations, prevents out-of-range pointer formation in child constructors, and tightens wbuf bounds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@simsong
simsong merged commit 1eeac2c into main Jul 20, 2026
12 checks passed
@simsong
simsong deleted the codex/fix-sbuf-contract branch July 20, 2026 03:48
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.

P0: Restore the sbuf_t bounds-checking contract

3 participants