Skip to content

libpcp: reject undersized metadata records in __pmLogLoadMeta - #2663

Draft
kurik wants to merge 2 commits into
performancecopilot:mainfrom
kurik:LogMeta
Draft

libpcp: reject undersized metadata records in __pmLogLoadMeta#2663
kurik wants to merge 2 commits into
performancecopilot:mainfrom
kurik:LogMeta

Conversation

@kurik

@kurik kurik commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

__pmLogLoadMeta() rejects undersized metadata records
qa/2009: exercise __pmLogLoadMeta undersized metadata record rejection

__pmLogLoadMeta() rejects undersized metadata records
qa/2009: exercise __pmLogLoadMeta undersized metadata record rejection

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 5717ef19-c678-4797-827a-52f4c6199f4c

📥 Commits

Reviewing files that changed from the base of the PR and between eed8605 and 314a899.

⛔ Files ignored due to path filters (1)
  • qa/2009.out is excluded by !**/*.out
📒 Files selected for processing (3)
  • qa/2009
  • src/libpcp/src/logmeta.c
  • src/libpcp3/src/logmeta.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • qa/2009

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved archive metadata validation to safely reject undersized or corrupted records.
    • Corrupted archives now report a clear “Corrupted record” failure instead of being processed incorrectly.
  • Tests

    • Added automated coverage for malformed metadata records and verified that valid archives continue to open successfully.

Walkthrough

Both metadata loaders now validate record sizes and payload structure. QA test 2009 creates malformed archives and verifies that pmdumplog reports Corrupted record while valid archives still open.

Changes

Metadata validation

Layer / File(s) Summary
Validate metadata record bodies
src/libpcp/src/logmeta.c, src/libpcp3/src/logmeta.c
Both __pmLogLoadMeta implementations validate descriptor, instance-domain, label, and text records. Invalid sizes, name fields, trailing bytes, and missing NUL terminators return PM_ERR_LOGREC.
Crafted archive regression coverage
qa/2009, qa/group
QA test 2009 creates malformed metadata records, checks valid archive loading, verifies Corrupted record output, and registers the test in the archive-local group.

Sequence Diagram(s)

sequenceDiagram
  participant pmdumplog
  participant __pmLogLoadMeta
  participant metadata_archive
  pmdumplog->>__pmLogLoadMeta: load metadata records
  __pmLogLoadMeta->>metadata_archive: read crafted record header and body
  metadata_archive-->>__pmLogLoadMeta: undersized or malformed payload
  __pmLogLoadMeta-->>pmdumplog: return PM_ERR_LOGREC and report Corrupted record
Loading

Possibly related PRs

Suggested reviewers: kmcdonell

Poem

A rabbit checks each record byte,
Bad lengths leave before first light.
Names and labels must fit just right,
Good archives pass the loading gate,
Corrupt records meet their fate.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: rejecting undersized metadata records in __pmLogLoadMeta.
Description check ✅ Passed The description directly matches the implementation and QA changes for rejecting and testing undersized metadata records.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch LogMeta

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/libpcp/src/logmeta.c`:
- Around line 821-827: Enforce remaining-length bounds before every metadata
field and string read in the loaders’ record-parsing paths, including the checks
around the rlen validation in src/libpcp/src/logmeta.c (lines 821-827) and
src/libpcp3/src/logmeta.c (lines 767-773), rejecting zero or undersized positive
payloads without crossing record or allocation boundaries. Extend qa/2009 (lines
63-75) with total lengths 12 through 19 to cover these cases.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a9f0fa1-1045-44b5-921d-664eb5655ea9

📥 Commits

Reviewing files that changed from the base of the PR and between 74a4a4f and eed8605.

⛔ Files ignored due to path filters (1)
  • qa/2009.out is excluded by !**/*.out
📒 Files selected for processing (4)
  • qa/2009
  • qa/group
  • src/libpcp/src/logmeta.c
  • src/libpcp3/src/logmeta.c

Comment thread src/libpcp/src/logmeta.c Outdated
Strengthen __pmLogLoadMeta() beyond the rlen < 0 guard: reject empty
payloads (rlen <= 0), track remaining body bytes for TYPE_DESC (including
name length vs MAXPATHLEN), and enforce type-specific minimum sizes for
INDOM/LABEL/TEXT before malloc/parse.  Extend qa/2009 to cover total
record lengths 12..19.

Co-authored-by: Cursor <cursoragent@cursor.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