libpcp: fix stack buffer overflow in __pmLogLoadMeta pmDesc name read - #2682
libpcp: fix stack buffer overflow in __pmLogLoadMeta pmDesc name read#2682xiejing-dev wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe metadata loader now rejects negative or oversized metric name lengths with ChangesMalformed PCP metadata handling
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qa/2110 (1)
45-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the negative-length input.
The fixture writes only
EVIL = 8192. It exercises thelen >= MAXPATHLENbranch but not thelen < 0branch added insrc/libpcp/src/logmeta.cat Line 889. Add a second malformed record with a signed length such as-1and run both commands against it.🤖 Prompt for 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. In `@qa/2110` around lines 45 - 66, Extend the malformed metadata fixture generation around the existing EVIL record to also create a record with a signed name length of -1, then write its corresponding archive files and run both validation commands against that fixture. Preserve the existing 8192-length case so both the len >= MAXPATHLEN and len < 0 branches in logmeta handling are exercised.
🤖 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 `@qa/2110`:
- Around line 68-76: Update the pmlogcheck and pminfo checks in qa/2110 to
capture each command’s exit status while still sending their stdout and stderr
through _filter. Validate both statuses against the expected results, propagate
failure for signals or unexpected success, and only assign status=0 after both
checks pass.
---
Nitpick comments:
In `@qa/2110`:
- Around line 45-66: Extend the malformed metadata fixture generation around the
existing EVIL record to also create a record with a signed name length of -1,
then write its corresponding archive files and run both validation commands
against that fixture. Preserve the existing 8192-length case so both the len >=
MAXPATHLEN and len < 0 branches in logmeta handling are exercised.
🪄 Autofix
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: 8049d0ea-6cc3-4e1b-a2ca-3376305d8bff
⛔ Files ignored due to path filters (1)
qa/2110.outis excluded by!**/*.out
📒 Files selected for processing (3)
qa/2110qa/groupsrc/libpcp/src/logmeta.c
A TYPE_DESC metadata record in a PCP archive stores each metric name as a
length-prefixed string. __pmLogLoadMeta() read the 4-byte length, byte-swapped
it with ntohl(), and used it directly as the count for
__pmFread(name, 1, len, f);
name[len] = '\0';
where name is a fixed char name[MAXPATHLEN] (4096-byte) stack buffer, with no
upper-bound check. A crafted archive setting the name length > 4096 overflows
the buffer and smashes the stack (observed: SIGABRT from __stack_chk_fail).
Add a bounds check that rejects len < 0 or len >= MAXPATHLEN with PM_ERR_LOGREC
("Corrupted record in a PCP archive").
QA: add test 2110 (crafts a malformed archive with an oversized pmDesc name
length and asserts pmlogcheck/pminfo reject it gracefully instead of crashing).
654bbf1 to
794a244
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
The CI failure appears unrelated to this PR: QA 338 crossed UTC midnight, causing inconsistent TODAY/YESTERDAY archive names. It looks like the failed job needs to be rerun. |
|
@xiejing-dev agreed - I've kicked it off again. The test has an existing 10 minute proximity-to-midnight guard, perhaps that's still too close somehow. |
2 participants
Pull Request Description
A TYPE_DESC metadata record in a PCP archive stores each metric name as a length-prefixed string. __pmLogLoadMeta() read the 4-byte length, byte-swapped it with ntohl(), and used it directly as the count for
where name is a fixed char name[MAXPATHLEN] (4096-byte) stack buffer, with no upper-bound check. A crafted archive setting the name length > 4096 overflows the buffer and smashes the stack (observed: SIGABRT from __stack_chk_fail).
Add a bounds check that rejects len < 0 or len >= MAXPATHLEN with PM_ERR_LOGREC ("Corrupted record in a PCP archive").
QA: add test 2110 (crafts a malformed archive with an oversized pmDesc name length and asserts pmlogcheck/pminfo reject it gracefully instead of crashing).
Verify
logmeta_overflow_check.sh
See the testing script above. It is similar to the QA/2110 change added in this commit.
Before the fix, run it :
After this fix，run it:
Result: the overflow is now caught -- the tools exit cleanly (code 1) with Corrupted record in a PCP archive instead of crashing.