fix(dbengine): validate extent disk size and fix uncompressed page bounds - #22324
Conversation
…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_lengthon 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.
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.
|
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.
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.
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!
…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_readresults are treated as errors, with clearer logs for invalid extents.rrdeng_valid_extent_disk_size()with min/max helpers; enforced at journal restore, open-cache add, extent flush, and disk reads; improved logging when skipping invalid extents.data_length - trailer; reject mismatched checksums.uv_fs_readreturns as errors; free buffers and mark I/O error.Written for commit 138338f. Summary will update on new commits. Review in cubic