fix(objectSampler): drop Deallocate on GetClassSignature error path#535
fix(objectSampler): drop Deallocate on GetClassSignature error path#535jbachorik merged 3 commits intomainDataDog/java-profiler:mainfrom muse/fix-object-sampler-uafDataDog/java-profiler:muse/fix-object-sampler-uafCopy head branch name to clipboard
Conversation
The error branch in ObjectSampler::recordAllocation called jvmti->Deallocate(class_name) regardless of whether GetClassSignature actually returned success. The JVMTI spec does not guarantee output buffers are populated on a non-JVMTI_ERROR_NONE return; the pointer value is unspecified, so passing it to Deallocate is unsafe and was observed to crash with SIGSEGV (PROF-14626 / dd-crashsig-1b6db147ca4415c7). Add a TEST_F-based regression suite that exercises recordAllocation with a mock jvmtiEnv covering: error+non-NULL sentinel (the UAF), error+NULL, success+NULL name, success+valid name, and the !_active short-circuit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI Test ResultsRun: #26116304322 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-05-19 18:50:32 UTC |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes an unsafe JVMTI Deallocate call in ObjectSampler::recordAllocation by ensuring the profiler never attempts to free class_name when GetClassSignature returns an error, and adds gtest coverage to prevent regressions.
Changes:
- Remove
jvmti->Deallocate(class_name)from theGetClassSignatureerror path inObjectSampler::recordAllocation. - Add a new
TEST_F-based gtest fixture that validatesDeallocateis called only on the success path across several error/success scenarios. - Add a small test-only friend accessor to control
ObjectSampler::_activeand callrecordAllocationfrom unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/objectSampler.cpp | Stops calling Deallocate on GetClassSignature failure to avoid freeing an unspecified pointer. |
| ddprof-lib/src/main/cpp/objectSampler.h | Adds a friend accessor class used by unit tests to reach internals safely. |
| ddprof-lib/src/test/cpp/objectSampler_ut.cpp | Introduces a JVMTI mock + TEST_F fixture covering Deallocate behavior for error/success paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rkennke
left a comment
There was a problem hiding this comment.
It seems weird that a JVM returns anything not-NULL in the failure case. But if that is what we observed then what else can we do? shrugs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@rkennke indeed, it is very unexpected. But we did collect crashes at that exact point and other explanations, like broken JVMTI env are not really holding, since this happens inside a JVMTI callback 🤷 |
What does this PR do?:
Removes the
jvmti->Deallocate(class_name)call from the error branch ofObjectSampler::recordAllocationinddprof-lib/src/main/cpp/objectSampler.cpp, and adds a TEST_F-based gtest fixture exercising the JVMTI mock for five error/success scenarios.Motivation:
The error branch called
Deallocatewheneverclass_namehappened to be non-NULL after a failingGetClassSignaturecall. The JVMTI spec does not guarantee output buffers are populated on a non-JVMTI_ERROR_NONEreturn, so the pointer value is unspecified and passing it toDeallocateis unsafe — it crashed with SIGSEGV injvmti_Deallocatereached viaJvmtiExport::post_sampled_object_alloc(PROF-14626 /dd-crashsig-1b6db147ca4415c7, tracer 1.62.0*, Linux, JDK 25).The success path Deallocate (still present at line 91) is unchanged:
class_nameis freed exactly once whenGetClassSignaturereturnsJVMTI_ERROR_NONE. The success branch's lookup-and-record flow is unaffected.Additional Notes:
The test file uses a
TEST_Ffixture (ObjectSamplerDeallocateTest) so each test gets its own JVMTI function table and Deallocate counter, andTearDownrestores the process-globalObjectSampler::_activeflag — addressing review feedback from the muse chorus about shared static state, global counters, and singleton-flag leakage between tests.How to test the change?:
Run the gtest suite:
Locally on macos-arm64: 15 tests pass (10 existing
normalizeClassSignaturecases + 5 newObjectSamplerDeallocateTestcases — error+non-NULL sentinel, error+NULL name, success+NULL name, success+valid name (Deallocate called exactly once), and the !_active short-circuit).For Datadog employees:
@DataDog/security-design-and-guidance.🤖 Generated with Claude Code