[native] Fix a JNI reference leak and drop new[] from jstring_array_wrapper - #12568
Open
simonrozsival wants to merge 1 commit into
Open
Conversation
…rapper `jstring_array_wrapper::operator[]` fetches the element's JNI reference on first access, but the UTF characters behind it are only fetched later, when something calls `get_cstr ()`. `jstring_wrapper::release ()` bailed out early whenever `cstr` was null, so an element that was indexed but never read kept its local reference until control returned to Java. Release the characters and the reference independently instead. The overflow storage used `new jstring_wrapper[]`/`delete[]`, which is where `operator new[]` and `operator delete[]` entered `host.cc`. Allocate the array with `malloc ()` and run the constructors and destructors explicitly. Placement new is a compile-time construct, so it does not pull anything in from libc++. This removes `_Znam` and `_ZdaPv` from `host.cc`, taking the CoreCLR libc++ reference count from 23 down to 21. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/native/common/include/runtime-base/jni-wrappers.hh — 🤖 ❌ error Native memory — len * sizeof (jstring_wrapper) can overflow on 32-bit ABIs (since… |
What changed in this PR
This PR updates the native JNI wrapper utilities to (1) correctly release JNI local references even when UTF chars were never requested, and (2) remove new[]/delete[] usage from jstring_array_wrapper to help reduce C++ runtime symbol dependencies as part of the “drop-libc++” effort.
Changes:
- Fix
jstring_wrapper::release()so it always releases the JNI reference, independently of whether UTF chars were fetched. - Replace
new[]/delete[]allocation injstring_array_wrapperwithmalloc()+ placement-new construction + explicit destruction. - Add
<new>include to support placement new.
| File | Description |
|---|---|
src/native/common/include/runtime-base/jni-wrappers.hh |
Fixes JNI local ref lifetime handling and replaces new[] with malloc() + placement new to reduce C++ runtime dependencies. |
Comment on lines
+170
to
174
| wrappers = static_cast<jstring_wrapper*> (std::malloc (len * sizeof (jstring_wrapper))); | ||
| abort_unless (wrappers != nullptr, "Failed to allocate the JNI string array wrapper"); | ||
| for (size_t i = 0; i < len; i++) { | ||
| new (&wrappers [i]) jstring_wrapper (); | ||
| } |
simonrozsival
added
the
drop-libcpp
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.
Part of the drop-libc++ effort. Stacked on #12560.
Split out of #12560, where it had been sitting alongside the DSO loader changes despite being unrelated to them.
jstring_array_wrapperhad two problems.A JNI local reference leak
jstring_wrapper::release ()bailed out early when it had no UTF chars to release:But
jstring_array_wrapper::operator[]fetches the array element's reference eagerly, whilecstris only populated on the firstget_cstr ()call. So any element that was indexed but never read kept its local reference until the frame was popped. The reference and the UTF chars have independent lifetimes, so they are now released independently.new[]/delete[]The wrapper allocated its elements with
new jstring_wrapper[len], which is where_Znamand_ZdaPvinhost.cc.ocame from. It now usesmalloc()with explicit placement construction and destruction.jstring_wrapperis not an implicit-lifetime type — it has a user-provided destructor — somalloc()alone cannot begin its lifetime; placement new is required. Its default constructor is private, withjstring_array_wrapperas a friend, which is what makes that legal here.Results
host.cc.orefsAll three runtime lanes build clean.