fix(objectSampler): drop Deallocate on GetClassSignature error path#535
Open
jbachorik wants to merge 3 commits into
Open
fix(objectSampler): drop Deallocate on GetClassSignature error path#535jbachorik wants to merge 3 commits into
jbachorik wants to merge 3 commits into
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>
Contributor
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 |
Collaborator
Author
|
@codex review |
Contributor
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
approved these changes
May 19, 2026
Contributor
rkennke
left a comment
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>
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
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.
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