libpcp, pmproxy: fix unbounded memory growth from duplicate label records#2659
libpcp, pmproxy: fix unbounded memory growth from duplicate label records#2659agenticode wants to merge 1 commit into
Conversation
…ords
addlabel() appends every label record presented to it onto the
per-(type, ident) chain in the archive context's label hash, with no
duplicate detection. This is unlike the sibling metadata paths:
addindom() and __pmLogAddDesc() both reject records that duplicate one
already stored (addindom() even documents why: identical indoms "very
common in multi-archive contexts"). Labels get the same duplicate
records but keep every copy for the life of the context.
pmproxy's discovery presents the same label records repeatedly during
normal operation:
- when an archive is first discovered, __pmLogLoadMeta() (called from
pmNewContext()) loads all label records into the context, and then
process_metadata() re-reads the same records from the .meta file and
adds them again via pmDiscoverInvokeLabelsCallBacks();
- pmlogger_daily rollover and multi-archive contexts re-present the
same records with identical (type, ident, timestamp, content).
Because discovery contexts are long-lived, the duplicate pmLabelSets
accumulate without bound. Observed on PCP 7.1.5 as pmproxy RSS climbing
~62MB -> 1.8GB over six days (roughly doubling each night at the
rollover) until the process aborted under memory exhaustion and was
restarted by systemd at an ever-higher baseline; valkey stayed flat, so
the growth was entirely in the daemon. bcc memleak attributed ~30MB of
outstanding allocations per 30-minute window to __pmLogLoadLabelSet()
reached via process_metadata(), ~150x the equivalent (de-duplicated)
indom path.
Give addlabel() the same exact-duplicate rejection addindom() has: when
an incoming labelset matches an existing entry at the same timestamp
(same instances, label counts and JSON payload), free the wrapper and
return PMLOGPUTINDOM_DUP. The two callers that own the decoded labelsets
on a non-store outcome -- __pmLogLoadMeta() and
pmDiscoverInvokeLabelsCallBacks() -- free them on the duplicate return,
mirroring how __pmLogLoadMeta() already disposes of a duplicate indom's
payload. Only byte-for-byte redundant records are dropped, so no
context's effective label state changes; a genuine label change at a
later timestamp is a different time slot and is preserved.
A standalone reproduction that re-presents one context labelset to
__pmLogAddLabelSets() 30000 times goes from +8832 kB RSS (301 bytes each,
0 rejected) before this change to +0 kB (1 stored, 29999 rejected) after.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add duplicate labelset comparison and suppression during archive metadata insertion, free discarded decoded payloads, and broaden discovery callback cleanup to release labelsets whenever ownership is not transferred. ChangesLabelset duplicate handling
Suggested reviewers: 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
🤖 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 989-1000: Update the labelset cleanup after addlabel in the
surrounding function: whenever addlabel returns nonzero, free labelsets because
ownership remains local, including error returns, then explicitly set labelsets
to NULL after freeing it. Preserve the existing duplicate handling that converts
PMLOGPUTINDOM_DUP to success, and ensure cleanup cannot free the same pointer
again at the end label.
🪄 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: 04e62cf2-ced0-4d27-8b86-149740154c79
📒 Files selected for processing (2)
src/libpcp/src/logmeta.csrc/libpcp_web/src/discover.c
| if (sts >= 0) { | ||
| sts = addlabel(acp, type, ident, nsets, labelsets, &stamp); | ||
| /* | ||
| * A duplicate labelset was not stored, so we own the | ||
| * decoded labelsets and must free them here (compare the | ||
| * PMLOGPUTINDOM_DUP handling for indoms above). | ||
| */ | ||
| if (sts == PMLOGPUTINDOM_DUP) { | ||
| pmFreeLabelSets(labelsets, nsets); | ||
| sts = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fix memory leak on error paths and nullify pointer after free.
If addlabel returns an error (e.g., sts < 0), labelsets is not freed and will leak when the function subsequently jumps to the end label. Similar to the updated logic in discover.c, you should free labelsets for all non-zero return values because ownership is not transferred.
Additionally, as per path instructions, pointers must be explicitly nullified after they are freed.
🛠️ Proposed fix
if (sts >= 0) {
sts = addlabel(acp, type, ident, nsets, labelsets, &stamp);
/*
* A duplicate labelset was not stored, so we own the
* decoded labelsets and must free them here (compare the
* PMLOGPUTINDOM_DUP handling for indoms above).
*/
- if (sts == PMLOGPUTINDOM_DUP) {
+ if (sts != 0) {
pmFreeLabelSets(labelsets, nsets);
- sts = 0;
+ labelsets = NULL;
+ if (sts == PMLOGPUTINDOM_DUP)
+ sts = 0;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (sts >= 0) { | |
| sts = addlabel(acp, type, ident, nsets, labelsets, &stamp); | |
| /* | |
| * A duplicate labelset was not stored, so we own the | |
| * decoded labelsets and must free them here (compare the | |
| * PMLOGPUTINDOM_DUP handling for indoms above). | |
| */ | |
| if (sts == PMLOGPUTINDOM_DUP) { | |
| pmFreeLabelSets(labelsets, nsets); | |
| sts = 0; | |
| } | |
| } | |
| if (sts >= 0) { | |
| sts = addlabel(acp, type, ident, nsets, labelsets, &stamp); | |
| /* | |
| * A duplicate labelset was not stored, so we own the | |
| * decoded labelsets and must free them here (compare the | |
| * PMLOGPUTINDOM_DUP handling for indoms above). | |
| */ | |
| if (sts != 0) { | |
| pmFreeLabelSets(labelsets, nsets); | |
| labelsets = NULL; | |
| if (sts == PMLOGPUTINDOM_DUP) | |
| sts = 0; | |
| } | |
| } |
🤖 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/logmeta.c` around lines 989 - 1000, Update the labelset
cleanup after addlabel in the surrounding function: whenever addlabel returns
nonzero, free labelsets because ownership remains local, including error
returns, then explicitly set labelsets to NULL after freeing it. Preserve the
existing duplicate handling that converts PMLOGPUTINDOM_DUP to success, and
ensure cleanup cannot free the same pointer again at the end label.
Source: Path instructions
|
@agenticode great analysis, thanks for looking into this! Could you take a look at the coderabbit review note and either dismiss as not-an-issue or update the code? Thanks! It's difficult to automate a memory leak test for a daemon. If there's no easy way to insert valgrind into this callchain (?) - some pmproxy tests manage it, worth a look - but if no, then relying on observing no regressions with the existing functional tests should be fine here. @lmchilton could you take a look too, ISTR you hacking in this area recently and its always good to have second opinion. |
2 participants
I've been running
pmproxywith the Valkey-backedpmseriesand grafana-pcp for retrospective analysis on a small, always-on box, and over the course of about a week I watched its RSS creep from ~60 MB past 1.8 GB — roughly doubling every night — until it eventually took aSIGABRTand got respawned by systemd, only to start climbing again from a higher floor. The key server stayed flat the whole time, so whatever was growing lived in the daemon itself, not in the stored series.A core dump plus a
memleak(bcc) pass over the live process kept landing on the same spot:__pmLogLoadLabelSet(), reached throughprocess_metadata()in the discovery module — tens of MB of outstanding allocations in a single half-hour window, and next to none of it ever released. The equivalent instance-domain path barely registered, and that contrast is what finally gave it away.What's happening
addlabel()inlogmeta.clinks every label record it's handed onto the per-(type, ident)chain in the archive context, and — unlike its two siblings — never checks whether that record is already there.addindom()filters exact duplicates (its own comment spells out why: identical indoms are "very common in multi-archive contexts"), and__pmLogAddDesc()returns early once the PMID is known. Only labels keep every copy.Harmless enough, if each record turned up once. It doesn't. Discovery re-presents the very same
(type, ident, timestamp, content)labels on perfectly ordinary paths:pmNewContext()→__pmLogLoadMeta()loads all of its label records into the context, and thenprocess_metadata()opens the.metafile directly and feeds the same records back throughpmDiscoverInvokeLabelsCallBacks();pmlogger_dailyrotation and multi-archive contexts hand the same records back again.Because discovery contexts are long-lived, those duplicate
pmLabelSets simply pile up. Nothing is technically lost — it all stays reachable from the context's label hash — so an ordinary valgrind leak-check comes back clean, and it only ever shows as growing RSS.The change
I gave
addlabel()the same exact-duplicate rejectionaddindom()already has: when an incoming labelset matches an existing entry at the same timestamp (same instances, label counts and JSON payload), free the wrapper and returnPMLOGPUTINDOM_DUPrather than chaining a second copy. The two callers that own the decoded labelsets when they're not stored —__pmLogLoadMeta()andpmDiscoverInvokeLabelsCallBacks()— free them on that return, mirroring how__pmLogLoadMeta()already disposes of a duplicate indom's payload.Only byte-for-byte redundant records get dropped, so no context's effective label state changes. A genuine label change at a later timestamp is a different time slot and is kept, exactly as for indoms.
Checking it
A small standalone program that re-presents one context labelset to
__pmLogAddLabelSets()30,000 times — a stand-in for the re-read paths above — makes it plain:pminfo --labelsover an archive gives byte-identical output before and after, so real label state is left untouched.Where I'd welcome a second look
A few things I'd rather have confirmed than assume:
Glad to add a
qa/test for this if that's wanted — I kept the reproduction standalone for the moment.