libpcp: fix heap buffer over-read in __pmLogLoadLabelSet - #2667
Open
lilu5458 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesLabelset decoding safety
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/e_labels.c`:
- Around line 268-271: Make the JSON bounds checks in src/libpcp/src/e_labels.c
lines 268-271 and src/libpcp3/src/e_labels.c lines 273-277 overflow-safe by
replacing the k + jsonlen > rlen arithmetic with a subtraction-based or
size_t-based comparison; apply the same validation logic at both sites before
processing untrusted sizes.
- Around line 245-249: The error exits in the label decoding logic leak
previously decoded label sets and nested allocations. Update the cleanup paths
in src/libpcp/src/e_labels.c at lines 245-249, 268-271, and 284-288, and in
src/libpcp3/src/e_labels.c at lines 250-254, 273-277, and 289-293, to use the
recursive labelset cleanup routine; include the current JSON allocation where it
exists and all earlier sets before returning the negative status.
- Around line 245-249: Add a minimum-header-length guard at the start of both
__pmLogLoadLabelSet implementations, before decoding any TYPE_LABEL_V2
timestamp, type, identifier, or nsets fields; update src/libpcp/src/e_labels.c
lines 245-249 and src/libpcp3/src/e_labels.c lines 250-254 consistently to
reject truncated records before allocation or field reads.
🪄 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: 517d341a-bbd4-4e83-b27e-9ff89e3388fe
📒 Files selected for processing (2)
src/libpcp/src/e_labels.csrc/libpcp3/src/e_labels.c
Comment on lines
+245
to
+249
| if (k + 2*sizeof(__int32_t) > rlen) { | ||
| sts = PM_ERR_LOGREC; | ||
| free(labelsets); | ||
| return sts; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use recursive labelset cleanup on every new error exit.
src/libpcp/src/e_labels.c#L245-L249: free all previously decoded sets, not onlylabelsets.src/libpcp/src/e_labels.c#L268-L271: free nested allocations from earlier sets before returning.src/libpcp/src/e_labels.c#L284-L288: include the current JSON allocation and all earlier sets.src/libpcp3/src/e_labels.c#L250-L254: apply the same cleanup before the current set is allocated.src/libpcp3/src/e_labels.c#L273-L277: free nested allocations from earlier sets.src/libpcp3/src/e_labels.c#L289-L293: include the current JSON allocation and all earlier sets.
The callers terminate on negative status, so they cannot repair these leaks.
📍 Affects 2 files
src/libpcp/src/e_labels.c#L245-L249(this comment)src/libpcp3/src/e_labels.c#L250-L254
🤖 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 `@src/libpcp/src/e_labels.c` around lines 245 - 249, The error exits in the
label decoding logic leak previously decoded label sets and nested allocations.
Update the cleanup paths in src/libpcp/src/e_labels.c at lines 245-249, 268-271,
and 284-288, and in src/libpcp3/src/e_labels.c at lines 250-254, 273-277, and
289-293, to use the recursive labelset cleanup routine; include the current JSON
allocation where it exists and all earlier sets before returning the negative
status.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)e_labels\.c$|logmeta\.c$' || true
echo
echo "== src/libpcp/src/e_labels.c outline/size =="
wc -l src/libpcp/src/e_labels.c || true
ast-grep outline src/libpcp/src/e_labels.c || true
echo
echo "== src/libpcp/src/e_labels.c lines 200-290 =="
sed -n '200,290p' src/libpcp/src/e_labels.c
echo
echo "== src/libpcp3/src/e_labels.c lines 200-305 =="
sed -n '200,305p' src/libpcp3/src/e_labels.c
echo
echo "== search for decoder caller context =="
rg -n "e_labels|labelsets|_logmeta|logmeta" src/libpcp src/libpcp3 -g '*.c' -g '*.h' || trueRepository: performancecopilot/pcp
Length of output: 22809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== e_labels.c relevant call sites =="
sed -n '150,178p' src/libpcp/src/e_labels.c
sed -n '180,198p' src/libpcp/src/e_labels.c
sed -n '180,198p' src/libpcp3/src/e_labels.c
echo
echo "== logmeta.c call sites around __pmLogLoadLabelSet =="
sed -n '950,1005p' src/libpcp/src/logmeta.c
sed -n '910,945p' src/libpcp3/src/logmeta.c
echo
echo "== search for length before addlabel / __pmLogLoadLabelSet =="
rg -n "addlabel|__pmLogLoadLabelSet|rlen" src/libpcp/src/logmeta.c src/libpcp3/src/logmeta.c src/libpcp/src/e_labels.c src/libpcp3/src/e_labels.c
echo
echo "== read-only header-size invariant probe =="
python3 - <<'PY'
from pathlib import Path
import re
def read_file(path):
return path.read_text()
def extract_decoder(path):
text=read_file(path)
m=re.search(r'int\s+__pmLogLoadLabelSet\([^;]*\{\s*(?P<body>.*?)\n(?=\nint\s|$)', text, re.S)
if not m:
# broader until first free(labelsets); and return sts after JSON allocation
lines=text.splitlines()
start=lines.index(next(l for l in lines if '__pmLogLoadLabelSet' in l))
body=[]
brace=0
for l in lines[start:]:
body.append(l)
brace += l.count('{') - l.count('}')
if brace == 0 or ('free(labelsets)' in l and 'return sts' in l):
break
return '\n'.join(body[:140])
return m.group('body')
for path in [Path('src/libpcp/src/e_labels.c'), Path('src/libpcp3/src/e_labels.c')]:
body=extract_decoder(path)
print(f'-- {path} --')
for line in body.splitlines()[:32]:
if kcheck in body:
print(line)
print('checks:', {
'early_header_check_before_type_id_read': 'k + 6*sizeof(__int32_t) > rlen' in body or 'k + 2*sizeof(__int32_t) + 3*sizeof(unsigned int)' in body,
'first_set_guard_present': 'if (k + 2*sizeof(__int32_t) > rlen)' in body,
'first_set_guard_k': body.find('if (k + 2*sizeof(__int32_t) > rlen)'),
'first_labelset_cast': body.find('inst = *((unsigned int*)&tbuf[k])'),
})
print(body[0:1200])
PYRepository: performancecopilot/pcp
Length of output: 10126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/libpcp/src/e_labels.c lines 30-210 =="
sed -n '30,210p' src/libpcp/src/e_labels.c
echo
echo "== src/libpcp3/src/e_labels.c lines 30-220 =="
sed -n '30,220p' src/libpcp3/src/e_labels.c
echo
echo "== logmeta label record length parsing around rlen =="
sed -n '740,830p' src/libpcp/src/logmeta.c
sed -n '760,830p' src/libpcp3/src/logmeta.cRepository: performancecopilot/pcp
Length of output: 14352
Guard rlen before decoding the label record header.
Both __pmLogLoadLabelSet implementations read TYPE_LABEL_V2 timestamps, type, identifier, and nsets before checking k + 2*sizeof(__int32_t) > rlen and allocate/clean up labelsets only after nsets validation. Add a minimum-header-length check at the start of the decoder in src/libpcp/src/e_labels.c and src/libpcp3/src/e_labels.c to reject truncated records before any header field reads.
📍 Affects 2 files
src/libpcp/src/e_labels.c#L245-L249(this comment)src/libpcp3/src/e_labels.c#L250-L254
🤖 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 `@src/libpcp/src/e_labels.c` around lines 245 - 249, Add a
minimum-header-length guard at the start of both __pmLogLoadLabelSet
implementations, before decoding any TYPE_LABEL_V2 timestamp, type, identifier,
or nsets fields; update src/libpcp/src/e_labels.c lines 245-249 and
src/libpcp3/src/e_labels.c lines 250-254 consistently to reject truncated
records before allocation or field reads.
Comment on lines
+268
to
+271
| if (k + jsonlen > rlen) { | ||
| free(labelsets); | ||
| return PM_ERR_LOGREC; | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Make both JSON bounds checks overflow-safe.
src/libpcp/src/e_labels.c#L268-L271: replacek + jsonlen > rlenwith a subtraction- orsize_t-based check.src/libpcp3/src/e_labels.c#L273-L277: apply the identical overflow-safe comparison.
As per path instructions, C/C++ code must check arithmetic on untrusted sizes.
📍 Affects 2 files
src/libpcp/src/e_labels.c#L268-L271(this comment)src/libpcp3/src/e_labels.c#L273-L277
🤖 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 `@src/libpcp/src/e_labels.c` around lines 268 - 271, Make the JSON bounds
checks in src/libpcp/src/e_labels.c lines 268-271 and src/libpcp3/src/e_labels.c
lines 273-277 overflow-safe by replacing the k + jsonlen > rlen arithmetic with
a subtraction-based or size_t-based comparison; apply the same validation logic
at both sites before processing untrusted sizes.
Source: Path instructions
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
1 participant
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.
Fix: heap buffer over-read in
__pmLogLoadLabelSetVulnerability
__pmLogLoadLabelSet()in bothsrc/libpcp3/src/e_labels.candsrc/libpcp/src/e_labels.ccopiesjsonlenbytes fromtbuf[k]viamemcpywithout checking thatk + jsonlen <= rlen. A malicious PCP archive can setjsonlento a value that exceeds the remaining buffer space, causing a heap buffer over-read of up toPM_MAXLABELJSONLEN(65535) bytes.Root cause
The function parses label set data from an archive metadata record. The offset
kadvances through the buffer as fields are read (timestamp, type, ident, nsets, inst, jsonlen, json data, nlabels, labels), butkis never checked againstrlenbefore readinginst,jsonlen, or before thememcpythat copiesjsonlenbytes of JSON data. Onlynlabelshas a partial bounds check (k + nlabels * sizeof(pmLabel) > rlen), but this check comes too late — after thememcpyhas already occurred.Attack vector
A user opens a malicious PCP archive file with
pmlogdump,pmlogextract, or any PCP tool that calls__pmLogLoadMeta(). The.metafile contains aTYPE_LABEL_V2record with ajsonlenfield larger than the record body.Impact
jsonbufferSIGSEGVif the over-read hits an unmapped pageFix
Add three bounds checks in the
nsetsloop:instandjsonlen: checkk + 2*sizeof(__int32_t) <= rlenmemcpyof JSON data: checkk + jsonlen <= rlennlabels: checkk + sizeof(__int32_t) <= rlenAll checks return
PM_ERR_LOGRECon failure, consistent with existing error handling in the function.Testing
TYPE_LABEL_V2record,jsonlen=1000,rlen=32SIGSEGV(exit 139) — heap over-read crosses guard pagepmlogdumpreports "Corrupted record in a PCP archive" (exit 1)Files changed
src/libpcp3/src/e_labels.c: +15 linessrc/libpcp/src/e_labels.c: +15 lines