Skip to content

[native] Fix a JNI reference leak and drop new[] from jstring_array_wrapper - #12568

Merged
simonrozsival merged 3 commits into
mainfrom
dev/simonrozsival/fix-jstring-array-wrapper
Sep 3, 2026
Merged

simonrozsival merged 3 commits into
mainfrom
dev/simonrozsival/fix-jstring-array-wrapper

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Aug 28, 2026 •

Copy link
Copy Markdown
Member

PR relationship: No code prerequisite; this PR is based directly on main. The absolute CoreCLR totals below were measured in the former serialized stack.

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_wrapper had two problems.

A JNI local reference leak

jstring_wrapper::release () bailed out early when it had no UTF chars to release:

if (cstr == nullptr) {
    return;
}

But jstring_array_wrapper::operator[] fetches the array element's reference eagerly, while cstr is only populated on the first get_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 _Znam and _ZdaPv in host.cc.o came from. It now uses malloc() with explicit placement construction and destruction.

jstring_wrapper is not an implicit-lifetime type — it has a user-provided destructor — so malloc() alone cannot begin its lifetime; placement new is required. Its default constructor is private, with jstring_array_wrapper as a friend, which is what makes that legal here.

Results

before after
host.cc.o refs 11 9
CoreCLR total refs 23 21

All three runtime lanes build clean.

Copilot AI lite review requested due to automatic review settings August 28, 2026 12:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

Review tier: Lite
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity 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 in jstring_array_wrapper with malloc() + 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 thread src/native/common/include/runtime-base/jni-wrappers.hh Outdated
@simonrozsival simonrozsival added the drop-libcpp Work to remove the libc++ dependency from Android NativeAOT label Aug 28, 2026
…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
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/fix-jstring-array-wrapper branch from 212b1fb to 98e08b1 Compare September 3, 2026 06:15
@simonrozsival
simonrozsival force-pushed the dev/simonrozsival/clr-drop-std-semaphore branch from e808135 to 54a7fa6 Compare September 3, 2026 06:15
@simonrozsival
simonrozsival changed the base branch from dev/simonrozsival/clr-drop-std-semaphore to main September 3, 2026 06:15
simonrozsival and others added 2 commits September 3, 2026 10:20
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]>
@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Sep 3, 2026
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12568

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

Comment thread src/native/common/include/runtime-base/jni-wrappers.hh
@simonrozsival
simonrozsival merged commit d252d01 into main Sep 3, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/fix-jstring-array-wrapper branch September 3, 2026 12:49
simonrozsival added a commit that referenced this pull request Sep 8, 2026
…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.
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants