fix(dbengine): validate extent disk size and fix uncompressed page bounds - #22324
fix(dbengine): validate extent disk size and fix uncompressed page bounds#22324stelfrag merged 3 commits into
Conversation
Sorry, something went wrong.
…unds Add rrdeng_valid_extent_disk_size() and check at journal restore, open-cache add, and disk read. Fix missing bounds check in the uncompressed page path that could read past payload_length on corrupt extent metadata. Treat short reads from uv_fs_read as errors.
There was a problem hiding this comment.
Pull request overview
This PR hardens DBEngine extent handling by validating extent sizes at multiple ingestion/read points, tightening bounds checks in the uncompressed-page path, and treating short disk reads as I/O errors to better handle corrupt on-disk metadata.
Changes:
- Add rrdeng_valid_extent_disk_size() (min/max helpers) and enforce it during journal restore, open-cache insertion, and extent reads.
- Fix missing bounds check for uncompressed pages to prevent reading past payload_length on corrupt extent metadata.
- Treat short reads from uv_fs_read() as errors (not just -1).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| src/database/engine/rrdengine.h | Introduces min/max extent disk size helpers and a shared validator. |
| src/database/engine/rrdengine.c | Adds an internal invariant check for flushed extent sizes and braces around open-cache insertion. |
| src/database/engine/pdc.c | Uses the new size validator, fixes CRC length source, adds uncompressed bounds check, validates extent reads, and treats short reads as errors. |
| src/database/engine/pagecache.c | Prevents adding open-cache entries for invalid extent sizes (with logging). |
| src/database/engine/journalfile.c | Validates extent_size during journal restore to reject corrupted transactions earlier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds validation and safety checks around on-disk extent sizes and page payload boundaries to prevent invalid reads and improve error handling for corrupted metadata / short I/O.
Changes:
- Introduces rrdeng_valid_extent_disk_size() and enforces it across restore, open-cache, flush, and disk read paths.
- Adds missing bounds checks for uncompressed page loads to avoid over-reading past payload_length.
- Treats short uv_fs_read() reads as errors (not just -1).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| src/database/engine/rrdengine.h | Adds min/max helpers and a shared extent size validator. |
| src/database/engine/rrdengine.c | Validates flushed extent size before adding pages to open cache. |
| src/database/engine/pdc.c | Validates extent size, fixes CRC length basis, adds uncompressed payload bounds checks, and treats short reads as errors. |
| src/database/engine/pagecache.c | Skips adding open-cache pages for invalid extent sizes with rate-limited logs. |
| src/database/engine/journalfile.c | Validates extent_size during journal restore to reject corrupted payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves rrdengine robustness against corrupt extent metadata by validating on-disk extent sizes across multiple paths, tightening page bounds checks to avoid overreads, and treating partial reads as I/O errors.
Changes:
- Added rrdeng_valid_extent_disk_size() helpers and enforced them during journal restore, open-cache insertion, extent flush, and disk reads.
- Fixed missing bounds checks in the uncompressed page path to prevent reads past payload_length.
- Treated short uv_fs_read() results as errors and adjusted CRC calculation length.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file| File | Description |
|---|---|
| src/database/engine/rrdengine.h | Adds min/max/valid helpers for extent disk size. |
| src/database/engine/rrdengine.c | Validates flushed extent sizes and guards open-cache additions. |
| src/database/engine/pdc.c | Validates extent sizes on read, fixes CRC length, strengthens page bounds checks, and treats short reads as errors. |
| src/database/engine/pagecache.c | Skips adding open-cache pages for invalid extent sizes. |
| src/database/engine/journalfile.c | Validates extent size during journal restore to reject corrupted transactions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Improves robustness of the DBEngine extent read/restore/open-cache pipeline by validating on-disk extent sizes, tightening page payload bounds checks (especially for uncompressed extents), and treating short disk reads as I/O errors to avoid parsing incomplete/corrupt data.
Changes:
- Add rrdeng_valid_extent_disk_size() (min/max helpers) and enforce it in journal restore, open-cache insertion, disk reads, and (internally) after extent flush.
- Harden extent parsing by fixing CRC coverage length and adding missing bounds checks for the uncompressed-page path to prevent payload overreads on corrupt metadata.
- Treat partial uv_fs_read() results as failures (not just negative returns), freeing buffers and marking I/O errors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file| File | Description |
|---|---|
| src/database/engine/rrdengine.h | Adds reusable min/max size helpers and a centralized extent-size validator. |
| src/database/engine/rrdengine.c | Adds an internal assertion to catch invalid extent sizes immediately after flush (debug/internal-checks aid). |
| src/database/engine/pdc.c | Uses extent-size validation during parsing and reads; fixes CRC length; adds missing bounds checks for uncompressed pages; treats short reads as errors. |
| src/database/engine/pagecache.c | Skips adding open-cache entries for extents with invalid on-disk size (with rate-limited logging). |
| src/database/engine/journalfile.c | Rejects journal restore payloads that reference extents with invalid size. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
thiagoftsm
left a comment
There was a problem hiding this comment.
After almost two hours running, no issues were found with data and netdata. LGTM!
Sorry, something went wrong.
…unds (#22324) * fix(dbengine): validate extent disk size and fix uncompressed page bounds Add rrdeng_valid_extent_disk_size() and check at journal restore, open-cache add, and disk read. Fix missing bounds check in the uncompressed page path that could read past payload_length on corrupt extent metadata. Treat short reads from uv_fs_read as errors. * fix(dbengine): improve logging for invalid extent size handling * fix(pdc): enhance boundary checks for page offsets and lengths (cherry picked from commit 1608f65)
Summary
Summary by cubic
Validates extent disk sizes across restore/open/read/flush paths and strengthens page boundary checks to prevent overreads on corrupt metadata. Short or partial uv_fs_read results are treated as errors, with clearer logs for invalid extents.
Written for commit 138338f. Summary will update on new commits. Review in cubic