Possible out-of-bounds Context read - #774
Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #33619870379 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-02 10:52:50 UTC |
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The 10-attribute cap keeps the JFR schema and event field count within Context storage. The checked accessors also reject invalid tag indexes.
🤖 Datadog Autotest · Commit fdfffbe · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Automated sphinx review — spot-check findings below, none HIGH/CRITICAL.
| // aborted the JVM above. On any build, a mismatched schema/field count would make | ||
| // this parse fail or throw - reaching here with samples already proves the fix. | ||
| JfrEvents liveObjectEvents = verifyEvents("datadog.HeapLiveObject", false); | ||
| assertTrue(liveObjectEvents.hasItems(), "expected datadog.HeapLiveObject samples"); |
There was a problem hiding this comment.
MEDIUM (robustness): The deterministic capping assertions run after a GC/timing-dependent assertion (liveObjectEvents.hasItems()). If no datadog.HeapLiveObject sample is produced on a given run, the test fails before ever checking that attributes= was capped at MAX_CONTEXT_SLOTS, so a flake and a real regression of the fix look identical.
Suggestion: reorder the method body so the deterministic jdk.ActiveSetting cap assertions run first, then assert liveObjectEvents.hasItems() last.
| // Always enable library trap to catch wasmtime loading and patch its broken sigaction | ||
| switchLibraryTrap(true); | ||
|
|
||
| if (args._context_attributes.size() > DD_TAGS_CAPACITY) { |
There was a problem hiding this comment.
LOW (completeness): The DD_TAGS_CAPACITY invariant is enforced at a single call site deep inside Profiler::start() (after switchLibraryTrap/startRefresher), not at the point where the list is built in Arguments::parse (arguments.cpp:322/325), which still accepts an unbounded number of names. Any future reader of args._context_attributes added earlier in start() — or any other entry point feeding JfrMetadata::initialize — would see the uncapped list again.
Suggestion: apply (or additionally apply) the cap in Arguments::parse where _context_attributes is populated.
| return i >= 0 && (u32)i < DD_TAGS_CAPACITY; | ||
| } | ||
| public: | ||
| u32 getTag(int i) { |
There was a problem hiding this comment.
LOW (consistency): getTag/setTag take a signed int index while both call sites (flightRecorder.cpp:2117 and threadLocalData.cpp:142) iterate with size_t, producing an implicit narrowing conversion on every call and requiring the extra i >= 0 half of isValidIndex. The neighboring, equivalent accessor ProfiledThread::getOtelTagEncoding (threadLocalData.h:404) already uses an unsigned index with a single-sided check.
Suggestion: take the index as u32 (or size_t) in both getTag/setTag and isValidIndex; isValidIndex then reduces to a single i < DD_TAGS_CAPACITY comparison.
|
|
||
| Tag get_tag(int i) { return tags[i]; } | ||
| static bool isValidIndex(int i) { | ||
| return i >= 0 && (u32)i < DD_TAGS_CAPACITY; |
There was a problem hiding this comment.
LOW (test-adequacy, mutation-testing): No test in this PR would detect removing the i >= 0 && part of the bounds check in isValidIndex.
Suggestion: add a test verifying isValidIndex rejects negative indices.
3 participants
What does this PR do?:
Fixes an out-of-bounds read in
Context::get_tag()(now bounds-checkedgetTag/setTag) that could be triggered by configuring more thanDD_TAGS_CAPACITY(10) custom context attributes via theattributes=profiler argument.Context::tagsis a fixedTag[DD_TAGS_CAPACITY]array, butattributes=accepted an unbounded number of names and_num_context_attributeswas set to that unbounded count with no cap.Recording::writeContextSnapshot()looped up tonumContextAttributes()calling the uncheckedget_tag(i), so configuring 11+ attributes read past the end ofContextinto adjacent memory on everydatadog.HeapLiveObjectevent.Profiler::start()now capsargs._context_attributesatDD_TAGS_CAPACITY(logging a warning when truncating) before it drives both the JFR metadata schema and_num_context_attributes, so the schema and the per-event write count always agree.Context::get_tag()/setTag()are replaced with encapsulated, bounds-checkedgetTag/setTagaccessors (privatetags[], sharedisValidIndex()helper) as defense in depth for any future caller.Motivation:
Security review flagged that
attributes=had no upper bound on the number of configured names, while the nativeContextstruct backing per-event tag storage is fixed-size. An out-of-bounds read on every liveness-tracking event can disclose adjacent native heap memory into the JFR recording, or crash the process (e.g. under ASan).Additional Notes:
Iterated based on review feedback:
Tighten API: replaced directtags[]access with private storage +getTag/setTag.Fix: gavesetTaga real runtime bounds check (not just a debug-onlyassert), matchinggetTag's existing clamp-to-default behavior.Dedup code: factored the repeated bounds condition out into a sharedisValidIndex()helper used by both accessors.How to test the change?:
Added
ddprof-test/.../com/datadoghq/profiler/TooManyContextAttributesTest.java, which configuresattributes=with 13 names (3 over capacity) together with liveness tracking (memory=...:L, the code path that exerciseswriteContextSnapshot/getTag), generates enough live allocation volume to producedatadog.HeapLiveObjectsamples, and asserts:jdk.ActiveSettingreports exactly the first 10 attribute names, confirming the list is capped rather than the requested 13.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!