Repository navigation
[native] Fix a JNI reference leak and drop new[] from jstring_array_wrapper - #12568
Conversation
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. |
…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 <[email protected]> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
212b1fb to
98e08b1
Compare
e808135 to
54a7fa6
Compare
Co-authored-by: Copilot App <[email protected]>
Reject invalid array lengths and out-of-range access instead of returning a mutable sentinel that cannot release assigned JNI references. Document reference and storage ownership and simplify reassignment. Co-authored-by: Copilot App <[email protected]>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
✅ LGTM — 0 errors, 0 warnings, 1 suggestion. The reference and UTF-character lifetimes are now correctly independent, overflow is checked before heap allocation, explicit construction/destruction is balanced, and all 44 CI checks passed. I left one inline suggestion for direct regression coverage of the lazy-reference and heap-storage paths.
Generated by Android PR Reviewer for #12568 · gpt56 · 73.7 AIC · ⌖ 9.03 AIC · ⊞ 25.7K
Comment /review to run again
…12571) Part of #12533: remove the remaining allocating C++ library types from the CoreCLR assembly store and its decompressed-assembly cache. <!-- stack-prerequisites --> All prerequisites (#12541, #12545, #12551, #12552, #12560, #12568, and #12570) have merged. This branch was rebuilt on `main` at `4b0cc8efa9`. The runtime changes are confined to `src/native/clr/host/assembly-store.cc`, with cache regression coverage and APK size baseline updates alongside them. The already-merged startup, timing, DSO-loader, and other prerequisite changes are no longer included in this PR's diff. Upstream's pthread-backed `lock_guard` is preserved. <!-- /stack-prerequisites --> ### Changes - Replace `std::deque<WriteRequest>` with an intrusive FIFO linked through `WriteRequest::next`. - Replace the queued request's `std::unique_ptr<uint8_t[]>` with an explicit `uint8_t *payload`. Allocate the request and payload separately with two `malloc` calls and release both with two `free` calls on every completed or discarded write. If payload allocation fails, release the request and skip the write. No trailing-storage pointer arithmetic or explicit over-alignment is needed. - Replace the cache directory and per-request `std::string` paths with a process-lifetime directory string and checked `snprintf` formatting. Requests store the descriptor index instead of a path; the writer reconstructs the destination from the immutable cache directory. Replace temporary-file name construction and matching with `snprintf` and `strstr`. - Use a scoped `CachePath` buffer for directory creation, reads, writes, and stale-file cleanup. Short paths remain on the stack; longer paths retry in an exactly sized `malloc` allocation that is freed on every exit. Formatting and allocation failures are logged and remain non-fatal for the optional cache, unlike the abort-on-failure `Util::format_with_retry` helper. - Allocate the decompression tracking array and assembly-name table with `calloc`, replacing their `unique_ptr`/`new[]` allocations. - Extend the existing device cache regression test with a Java Application that supplies a code-cache directory longer than 1 KB. Cover persistence, mapping, corruption recovery, and stale temporary-file cleanup for both ordinary and long paths. The asynchronous writer, queue byte limit, cache footer validation, and scope-based pthread locking are retained. `Util::LocalPathBufferSize` is the stack fast-path size, not a hard path limit; truncated paths are never used for I/O. The change does not alter linker flags or other runtime hosts. ### Original measurements These results were recorded on the original pre-rebuild branch, **not on the current revision**: | Release, arm64 | Before | After | |---|---:|---:| | Undefined libc++ references in `assembly-store.cc.o` | 12 | 0 | | Undefined libc++ references across the CoreCLR host | 12 | 0 | | `libnet-android.release.so` size | 520,112 bytes | 203,776 bytes | Reaching zero references allowed the linker to stop pulling members from `libc++_static.a`. Relinking without libc++ also succeeded on that original branch; dropping the linker flag remains a follow-up. ### Current validation status The malloc fallback passed sanitizer-backed host cases covering stack/heap boundaries, nested lifetimes, allocation failure, and formatting/retry failures. The cache namespace extracted unchanged from the current source, using the real CRC32 and mutex helpers with runtime configuration/property stubs, compiled with the Android NDK for arm64, arm32, and x86_64. A standalone arm64 emulator harness passed cache initialization, asynchronous persistence, mmap reads, corruption recovery, and stale-file cleanup with both short and greater-than-1-KB paths. The generated Java regression-test application also compiled against the Android API. `git diff --check` passes. A local `dotnet build src/native/native-clr.csproj` restored packages but could not reach native compilation because this worktree lacks `xa-prep-tasks.dll` and `Xamarin.Android.Tools.BootstrapTasks.dll`. The complete native host build and repository device regression suite have **not** been run for this revision; the locally built SDK is unavailable. Binary measurements have not been repeated. The original branch's reported full native builds and binary measurements remain historical, not validation of this revision.

Part of the drop-libc++ effort.
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.