bound v2 journal header offsets before crc reads in list checks - #22666
Merged
stelfrag merged 1 commit intoJun 12, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Copy link
Copy Markdown
|
|
Sorry, something went wrong.
Copy link
Copy Markdown
|
|
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
sequenceDiagram
participant Loader as Journal Loader
participant Header as Journal v2 Header
participant ExtentCheck as journalfile_check_v2_extent_list()
participant MetricCheck as journalfile_check_v2_metric_list()
participant Mapping as Memory Mapping
participant CrcEngine as CRC Engine
Note over Loader,CrcEngine: V2 Journal Validation at Agent Startup
Loader->>Header: Read on-disk header (data_start, file_size)
Loader->>ExtentCheck: journalfile_check_v2_extent_list(data_start, file_size)
Loader->>MetricCheck: journalfile_check_v2_metric_list(data_start, file_size)
Note over ExtentCheck,CrcEngine: Bound check before any CRC read
ExtentCheck->>extentOffset: Read extent_offset from header
ExtentCheck->>extentCount: Read extent_count from header
ExtentCheck->>extentTrailerOff: Read extent_trailer_offset from header
alt Extent offset beyond file_size
ExtentCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Extent count would overflow bounds (division check)
ExtentCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Trailer offset out of range
ExtentCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Trailer not immediately after list (layout mismatch)
ExtentCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else All bounds pass
ExtentCheck->>Mapping: Access mapped memory within [data_start, data_start+file_size)
ExtentCheck->>CrcEngine: CRC over extent list bytes
CrcEngine-->>ExtentCheck: CRC result
alt CRC matches trailer
ExtentCheck-->>Loader: Return 0 (valid)
else CRC mismatch
ExtentCheck-->>Loader: Return 1 (invalid, trigger rebuild)
end
end
Note over MetricCheck,CrcEngine: Same bound pattern for metric list
MetricCheck->>metricOffset: Read metric_offset from header
MetricCheck->>metricCount: Read metric_count from header
MetricCheck->>metricTrailerOff: Read metric_trailer_offset from header
alt Metric offset beyond file_size
MetricCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Metric count overflow (division check)
MetricCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Trailer offset out of range
MetricCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else Trailer not immediately after list
MetricCheck-->>Loader: Return 1 (invalid, trigger rebuild)
else All bounds pass
MetricCheck->>Mapping: Access mapped memory within safe range
MetricCheck->>CrcEngine: CRC over metric list bytes
CrcEngine-->>MetricCheck: CRC result
alt CRC matches trailer
MetricCheck-->>Loader: Return 0 (valid)
else CRC mismatch
MetricCheck-->>Loader: Return 1 (invalid, trigger rebuild)
end
end
Note over Loader: On any return 1 -> Journal marked invalid, rebuilt from WAL
Sorry, something went wrong.
jmestwa-coder
force-pushed
the
bound-v2-journal-header-offsets
branch
from
3621666 to
cfa6b6e
Compare
June 10, 2026 14:52
Copy link
Copy Markdown
|
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
Done |
Sorry, something went wrong.
stelfrag
approved these changes
Jun 12, 2026
Merged
Ferroin
pushed a commit
that referenced
this pull request
Jul 15, 2026
(cherry picked from commit 39765e2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
journalfile_check_v2_extent_list() runs on every v2 journal load (before the db_engine_journal_check gate) and reads extent_count * sizeof(struct journal_extent_list) bytes from extent_offset, plus the trailer at extent_trailer_offset, all taken straight from the on-disk header with no bound against the mapping. The file-trailer crc only covers the header bytes and is not a MAC, so a crafted or corrupted header passes it; the read then walks past the mmap, and since PROTECTED_ACCESS_SETUP only covers [data_start, data_start + file_size), an offset past that range faults outside the protected region and aborts with SIGBUS. On 32-bit the count * sizeof length also wraps. The file_size argument was already threaded into both helpers but ignored (UNUSED).
Wire file_size into journalfile_check_v2_extent_list() and journalfile_check_v2_metric_list(): bound the offset first so the subtraction can't underflow, bound the count by division instead of multiplying it out, and require the trailer to sit immediately after the list per the on-disk layout. Out-of-range headers return 1 and are treated as invalid (rebuild), same as a crc mismatch. Same shape as the inline checks added to the populate walk in #22514.
Test Plan
Reproduced with a standalone harness that places a v2 journal mapping in front of a PROT_NONE guard page and sets extent_count = 0x08000000: the current read spans bytes [64 .. 2147483712) against a one-page mapping and faults with SIGBUS; the bounded version returns 1 before any read. CI covers the existing dbengine journal load and migration paths.
Additional Information
The two helpers are reached from journalfile_v2_validate() (extent check always, metric check under db_engine_journal_check) and from journalfile_v2_populate_retention_to_mrg() where the metric check already has an inline guard. The added checks make the helpers safe on their own regardless of caller.
For users: How does this change affect me? Affects the dbengine at agent startup. No visible change for healthy databases. A truncated or corrupted v2 journal that could previously crash the agent during load is now rejected and rebuilt.Summary by cubic
Bounds checks v2 journal extent and metric list offsets and counts before CRC reads to avoid out‑of‑mapping access. Corrupted or truncated headers now fail validation and trigger a safe rebuild instead of crashing on startup.
Written for commit cfa6b6e. Summary will update on new commits.