Fix buffer overrun in livenessTracker.cpp#555
Conversation
Adds comprehensive test coverage for the fix in commit 72c0e14 that prevents _table_cap from being updated when realloc fails. The test validates that capacity is only updated after successful reallocation, preventing buffer overrun vulnerabilities. Test includes 5 cases covering correct resize behavior, the buggy pattern that was fixed, and edge cases with capacity limits. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
CI Test ResultsRun: #26600478483 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-amd64/debug / 25-graalJob: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-05-28 20:47:31 UTC |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 516fef26b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| Log::debug("Cannot add sampled object to Liveness tracking table, " | ||
| "resize attempt failed, the table is overflowing"); | ||
| Log::debug("Cannot add sampled object to Liveness tracking table, " | ||
| "resize attempt failed, the table is overflowing"); | ||
| } |
There was a problem hiding this comment.
When realloc returns null here and cleanup_table(true) did not free a slot, the subsequent goto retry re-enters with the unchanged _table_cap; the second insertion attempt falls through with idx == _table_cap and never stores or deletes the jweak created at the start of track. Under sustained full-table/OOM conditions this leaks one weak global reference per sampled object, so the failed-resize path should delete ref when the retry cannot insert it.
Useful? React with 👍 / 👎.
| bool success = mock.resizeTableBuggy(20); | ||
| EXPECT_TRUE(success); | ||
| EXPECT_EQ(mock.getCapacity(), 20); |
There was a problem hiding this comment.
This test only calls resizeTableBuggy(20) on a normal small allocation, so the pre-fix pattern succeeds and the test would still pass; the rest of the failure scenario is comments with no assertion or mocked failed realloc. Because AGENTS.md requires bug-fix tests to fail before the fix, this should force a realloc failure and assert that capacity remains unchanged, otherwise the test cannot catch a future reintroduction of _table_cap = newcap before checking tmp.
Useful? React with 👍 / 👎.
c712e11
into
main
What does this PR do?:
The fix addresses a critical bug in ddprof-lib/src/main/cpp/livenessTracker.cpp:354 where _table_cap was being updated before verifying that the realloc() call succeeded.
Motivation:
Fix possible memory corruption.
Additional Notes:
The original code assigned _table_cap = newcap inside the realloc() call itself:
TrackingEntry *tmp = (TrackingEntry *)realloc(
_table, sizeof(TrackingEntry) * (_table_cap = newcap));
If realloc() failed and returned nullptr, _table_cap would still be updated to the larger value, creating a mismatch between the recorded capacity and the actual allocated memory size. Subsequent operations would then write beyond the actual buffer bounds, causing a buffer overrun.
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!