Uh oh!
There was an error while loading. Please reload this page.
Replace simple CLR local strings - #12517
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the effort to remove straightforward uses of the native local-string hierarchy in the CLR host/runtime by switching to fixed-size buffers (with snprintf) and by using a fixed-buffer system property retrieval API that rejects undersized buffers to avoid truncation.
Changes:
- Added a
monodroid_get_system_property(std::string_view, char*, size_t)overload and updated fixed-array callers to write directly into their buffers. - Reworked primary override directory path formatting to use
snprintfinto fixed buffers. - Replaced debug typemap full-name composition with fixed-buffer
snprintfformatting.
Show a summary per file
| File | Description |
|---|---|
| src/native/clr/runtime-base/android-system-shared.cc | Adds fixed-buffer monodroid_get_system_property overload that rejects undersized buffers and avoids truncation. |
| src/native/clr/include/runtime-base/android-system.hh | Switches property callers to fixed buffers and formats primary override dir using snprintf. |
| src/native/clr/host/typemap.cc | Uses snprintf into a fixed buffer to build managed type debug names for typemap lookup. |
| src/native/clr/host/assembly-store.cc | Uses a fixed property buffer for debug.net.asmcache instead of dynamic_local_property_string. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
c111658 to
3a05311Compare2a03078 to
cda8d20Comparecda8d20 to
2e9c443Compare6d947da to
ddae8f9Compareddae8f9 to
4478d1cCompare4478d1c to
ce12492Comparece12492 to
85f0111Compare85f0111 to
8c8f213Compare4a079b8 to
fb358cfComparefb358cf to
ea5dfcbCompareThere was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 1 warning, 0 suggestions.
The fixed-buffer refactoring is focused, and the retrying path-format changes preserve dynamic growth where expected. However, bundled system-property values can exceed the Android platform limit, so rejecting them at the new fixed boundary silently changes existing logging, profiling, and timing behavior.
CI is not green: Package Tests macOS > Tests > APKs 1 is failing, and the aggregate dotnet-android check had not completed. Deeper failure details were unavailable because Azure DevOps requires authentication.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12517 · gpt56 · 347.6 AIC · ⌖ 8.93 AIC · ⊞ 25.7K
Comment /review to run again
Uh oh!
There was an error while loading. Please reload this page.
Addresses review feedback: the fixed-buffer overload returned -1 when a bundled (build-time) property value did not fit into the caller's buffer, so long values were treated as if the property were not set at all. The `dynamic_local_string` based overload it replaced grew onto the heap and had no such limit. Bundled properties come from `@(AndroidEnvironment)` files and are stored as NUL-terminated strings in static application data, so they are neither subject to Android's 92 byte property limit nor in need of copying. Return a `std::string_view` instead of an `int`: for Android system properties it views the caller's scratch buffer, for bundled properties it points directly at the application data, which restores the previous behaviour and avoids a copy. `FastTiming::parse_options()` used to tokenize its argument in place, which is not safe for a view over static data, so it now parses without mutating. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
…perty
The previous commit changed `monodroid_get_system_property ()` to return a
`std::string_view` so that bundled properties, whose length is not limited by
`PROPERTY_VALUE_BUFFER_LEN`, could be returned without copying them into the
caller's scratch buffer.
That works, but `std::string_view` deliberately makes no promise about
NUL-termination, while every value this function can return happens to be
NUL-terminated: `__system_property_get ()` terminates what it writes, and
bundled properties are NUL-terminated strings in static application data. The
header had to document that invariant in a comment ("The returned value is
always NUL-terminated") precisely because the type denies it, and callers such
as `get_max_gref_count_from_system ()` silently relied on it by passing
`.data ()` to `strtol ()` and to a `%s` format specifier.
Return `const char*` instead (and `nullptr` when the property is not set). The
lifetime rule is unchanged and still uniform - the result is valid for at least
as long as the caller's buffer - but NUL-termination is now guaranteed by the
type rather than by a comment, so `.data ()` no longer has to be laundered
through a `std::string_view`. Callers that need to tokenize the value construct
a `std::string_view` explicitly, which is honest about what they are doing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e- `format_managed_type_name ()` builds the name with a single `snprintf ()` instead of three `memcpy ()` calls and hand-rolled length arithmetic. The negative-required-capacity retry contract is unchanged, and the helper is now guarded by `#if defined (DEBUG)` like its only caller, which removes the unused-function warning this pull request introduced. - `FastTiming::parse_options ()` takes a `const char*` again and tokenizes with `strchr ()`/`strncmp ()`/`strtoull ()`. It had been rewritten around `std::string_view`, which added a C++ layer to code that was already plain C. The parser still cannot NUL-terminate in place - the value may point at immortal bundled property data - so each parameter is bounded by its length instead. The `duration=` and `filename=` edge cases behave as they did before. - The property lookup chain (`monodroid_get_system_property ()`, `monodroid__system_property_get ()` and `lookup_system_property ()`) takes `const char *name`, matching the other overloads. Previously it took a `std::string_view` and immediately called `.data ()` on it, which is the same NUL-termination laundering that motivated changing the return type. This also lets `HostEnvironment::lookup_system_property ()` use `strcmp ()` directly and drops `<string_view>` from `android-system-shared.cc` entirely. - Shorten the comments added by this pull request, and drop the `strings.hh` include from `logger.cc`, which no longer uses `dynamic_local_string`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`Logger::init_logging_categories ()` split the property value with `std::string_view`, and `set_category ()`, `set_log_file ()` and `open_file ()` took views as well. Tokenize the value with `strchr ()`/`strncmp ()` instead and pass the parameters around as a pointer and a length. This also removes a subtle NUL-termination assumption: `open_file ()` called `unlink (path.data ())`, which is only correct because every caller happened to pass a view over a NUL-terminated buffer. It now takes a `const char*`. A single `param_matches ()` helper does all of the comparisons, so the parameters no longer have to be NUL-terminated in place - the value may point at immortal bundled property data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The function returns either the caller's scratch buffer or a pointer into application data, which is easy to mistake for the "stack buffer or malloc" convention used elsewhere in this header, where the caller has to free the result when it differs from the buffer it passed in. Say so explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`Util::create_public_directory ()`, `Util::monodroid_fopen ()` and `Util::set_world_accessable ()` each took a `std::string_view` and immediately called `.data ()` on it to hand the path to `mkdir ()`, `fopen ()` or `chmod ()`. That is only correct because every caller happens to pass a view over a NUL-terminated buffer, which nothing enforces. Take a `const char*` instead, which is what these functions actually need. `Logger::open_file ()` and `Logger::init_reference_logging ()` follow, so `logger.cc` no longer refers to `std::string_view` at all and the `"..."sv` literals are gone with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
Address issues found while reviewing the C string conversion of the system property APIs. `AndroidSystem::lookup_system_property()` returned `prop_iter->first`, which is the map *key* -- the property name -- rather than its value, while reporting `prop_iter->second.length ()` as the length. In a DEBUG build with bundled `@(AndroidEnvironment)` properties, callers such as `Logger::init_logging_categories()` and `get_max_gref_count_from_system()` therefore parsed the property name instead of the value. Return `prop_iter->second` instead. `monodroid__system_property_get()` had a fallback branch that copied through a heap buffer whenever the caller's buffer was smaller than `PROPERTY_VALUE_BUFFER_LEN`. Its only caller now rejects that case before calling, so the branch was dead -- and it wrote a terminating NUL one byte past the end of the caller's buffer. Remove it, along with the now-unused `sp_value_len` parameter. This also drops a `new[]`/`delete[]` pair, removing two more libc++ references from the object file. Passing an undersized buffer to `monodroid_get_system_property()` was reported as `nullptr`, indistinguishable from an unset property. It is a programming error, so `abort_unless()` on it instead. Finally, derive the "gref="/"lref=" prefix length with `sizeof()` rather than hardcoding it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
commented
Aug 28, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
❌ Reject
Found 1 error and 0 warnings. The Mono DEBUG bundled-property lookup still returns the property name rather than its value through the newly added no-copy getter; this needs to be corrected for parity with the CLR fix in this PR.
The buffer lifetime contracts and exact-size fallback paths are otherwise well documented and consistently cleaned up. CI build 1572360 is still in progress; completed build and tooling lanes are passing with no reported failures so far.
Generated by Android PR Reviewer for #12517 · gpt56 · 331 AIC · ⌖ 9.09 AIC · ⊞ 25.7K
Comment /review to run again
Uh oh!
There was an error while loading. Please reload this page.
… lookup `AndroidSystem::lookup_system_property (name, value_len)` returned `p->name` while reporting `p->value_len` as its length, so every bundled `debug.*` property in a MonoVM Debug build resolved to its own name instead of its configured value. When the value was longer than the name this also read past the end of the name buffer. The CoreCLR implementation already returns the value; this brings Mono to parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
commented
Aug 28, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
❌ Reject
Found 1 error, 0 warnings, and 0 suggestions. The new pointer-returning property API correctly avoids copying bundled values, but get_max_gref_count_from_system() no longer distinguishes an empty override from a non-empty one; that can set the CoreCLR GREF threshold to zero and force repeated full collections.
CI is still running; the completed checks observed so far passed, with no failure reported at review time.
Generated by Android PR Reviewer for #12517 · gpt56 · 242.5 AIC · ⌖ 8.86 AIC · ⊞ 25.7K
Comment /review to run again
Uh oh!
There was an error while loading. Please reload this page.
`monodroid_get_system_property()` returned a non-null pointer for a
bundled property that exists with an empty value, where the previous
`dynamic_local_string` overload reported a length of 0 and callers
skipped it.
`get_max_gref_count_from_system()` then ran `strtol ("")` and set the
max JNI global reference count to 0 instead of keeping the 51200
default, and `create_update_dir()` treated an empty `debug.mono.profile`
as a request to create the `.__override__` directory.
The real-property path already behaves this way -- `__system_property_get`
returns 0 for a property that is absent *and* for one set to an empty
string -- so the bundled path now matches it rather than fixing each
call site.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e* `Logger::open_file()` now bails out when `override_dir` is null or empty instead of passing it to `Util::create_public_directory()` and `Util::join_paths()`. The latter takes `std::string_view`, and constructing one from a null `const char*` is undefined behaviour. The old `std::string_view const&` signature made this impossible; `const char*` does not. * MonoVM's `monodroid_get_system_property()` now aborts on a too-small scratch buffer, matching the CoreCLR implementation. Silently returning `nullptr` made a programming error indistinguishable from "property not set". * `format_primary_override_dir()` checked `buffer == nullptr` only after already passing `buffer` to `snprintf()`. Assert up front. * Fix stray indentation on the `Logger::set_category()` declaration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`monodroid_get_system_property()` claimed that a returned bundled property value "lives as long as the process". That is only true for NativeAOT, where the value points into the immortal app-environment blob. In Debug builds: * CoreCLR stores bundled properties in a `std::unordered_map<std::string, std::string>`, so re-assigning a property replaces the mapped `std::string`. * MonoVM's `add_system_property()` explicitly `free()`s `p->value` before installing the replacement. In both cases a previously returned pointer dangles. No current caller retains the pointer past the call, but the comment invited them to, so document the real guarantee instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
* `format_primary_override_dir()` passed `home.get_cstr ()` straight to `snprintf()`'s `%s`. `jstring_wrapper::get_cstr()` returns `nullptr` for a null `jstring`, and passing `nullptr` to `%s` is undefined behaviour. Abort with a clear message instead: an application without a files directory cannot work regardless. * `HostEnvironment::lookup_system_property()` left the `value_len` out-parameter untouched on both of its `nullptr` return paths, so the NativeAOT implementation of `AndroidSystem::lookup_system_property()` returned an uninitialized length while the CoreCLR one zeroed it first. The only caller happens to be safe because `||` short-circuits on the null pointer, but the two implementations of the same declaration should not disagree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`format_joined_path()`, `format_primary_override_dir()` and `format_managed_type_name()` all share one protocol: write into the supplied buffer and return either the formatted length or the negative required capacity. Each had its own open-coded "try the stack buffer, malloc and retry on failure" dance, two of which this branch had just introduced. Hoist that into `Util::format_with_retry()` and use it in all three places. This removes ~25 lines of triplicated code and, more importantly, leaves a single copy of the logic that has to get the retry, the abort-on-OOM and the `!= stack_buffer` ownership check right. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
commented
Aug 28, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Reviewed the current 18-file native-runtime diff against the repository’s native C++, JNI/interop, security, and compatibility rules. I found no new actionable defects in this revision; the earlier long bundled-property, Mono property value/name, and empty-value regressions are addressed by the latest commits.
CI is still in progress on Azure DevOps build #1572876: the completed checks are passing, with five checks still running and the aggregate check queued. No failing check is currently reported.
Generated by Android PR Reviewer for #12517 · gpt56 · 222.8 AIC · ⌖ 8.77 AIC · ⊞ 25.7K
Comment /review to run again
| if (p != nullptr) { | ||
| value_len = p->value_len; | ||
| return p->name; | ||
| return p->value; |
There was a problem hiding this comment.
how long has this been wrong? 👀
Uh oh!
There was an error while loading. Please reload this page.
Closes#12139Closes#12146 Removes `libc++_static.a`, `libc++abi.a` and `libunwind.a` from the NativeAOT link. This is the payoff for the local-string removal work in the rest of this stack: with `strings.hh`, `dynamic_local_string` and `static_local_string` gone, nothing in `libnaot-android` uses libc++ any more. For 32-bit ARM, ILC output still references the EHABI personality routines `__aeabi_unwind_cpp_pr0` and `__aeabi_unwind_cpp_pr1`. The NativeAOT runtime already contains their implementations in `libRuntime.WorkstationGC.a`, but dotnet/runtime localizes those symbols. `LinkNativeAotSharedLibrary` now makes an intermediate copy of that archive and promotes `pr0`, `pr1` and `pr2` to weak globals with `llvm-objcopy`. This adds no shim implementation and avoids linking or shipping a second copy of libunwind. ### Who was actually using the C++ runtime? Linking with libc++ removed leaves exactly six undefined symbols. Only one of them came from our code: | Symbol | Referenced from | | --- | --- | | `operator new[](size_t)` | `android-system-shared.cc` — **ours** | | `operator delete(void*)` | `gcenv.ee.cpp`, `UnixNativeCodeManager.cpp` | | `operator delete[](void*)` | `gcenv.ee.cpp`, `interoplibinterface_java.cpp` | | `operator new(size_t, nothrow_t const&)` | `gcenv.ee.cpp`, `TypeManager.cpp` | | `operator new[](size_t, nothrow_t const&)` | `gcenv.ee.cpp`, `RhConfig.cpp` | | `std::nothrow` | `gcenv.ee.cpp`, `UnixNativeCodeManager.cpp` | The five runtime-owned ones belong to the NativeAOT runtime from dotnet/runtime, and the ILC SDK already ships definitions for all of them in `libstdc++compat.a`. Our targets were unconditionally *removing* that archive with the comment *"This library conflicts with static libc++"* — which is only true while libc++ is linked. With libc++ gone there is no conflict, so we simply stop removing it. The one symbol that was ours came from a dead code path in `monodroid__system_property_get`, which is now removed earlier in the stack by #12517 (the PR that makes that path unreachable in the first place). The result is **no shim implementation** in this repo and a link with zero undefined symbols. ### 32-bit ARM EHABI `android-arm` uses ARM EHABI unwind tables. The generated NativeAOT object references `__aeabi_unwind_cpp_pr0` and `__aeabi_unwind_cpp_pr1`; removing the NDK `libunwind.a` initially exposed these as XA3007 linker failures on both macOS and Windows CI. `libRuntime.WorkstationGC.a` already carries the same `pr0`, `pr1` and `pr2` implementations in `Runtime.PrivateLibunwind.o`, together with the private unwinder they call. They are local symbols, so the application object cannot resolve them directly. Before linking an `armeabi-v7a` application, the build task creates `libRuntime.WorkstationGC.arm-ehabi.a` in the intermediate directory and promotes only those three symbols to weak globals. Weak binding preserves compatibility with applications that provide their own strong EHABI personalities, while the app export script keeps them local to the final DSO. No extra unwind code is linked: this reuses the object that the NativeAOT runtime already extracts. ### Size impact Default MAUI app (`dotnet new maui`), `net11.0-android`, `android-arm64`, Release, `PublishAot=true`. Both sides built clean from the same tree. | | before | after | delta | | --- | ---: | ---: | ---: | | `.so` | 25,229,320 | 25,029,728 | **−199,592** (−0.79%) | | `.so` deflated in APK | 9,829,143 | 9,761,291 | **−67,852** (−0.69%) | | APK | 14,903,624 | 14,833,992 | **−69,632** (−0.47%) | ### Commits 1. **Stop linking libc++ and libunwind** — the targets change. 2. **Update stale libc++ linker comments.** 3. **Stop shipping libc++ archives in the runtime packs** — see below. 4. **Reuse private ARM unwind personalities** — promote the runtime's existing EHABI symbols instead of restoring `libunwind.a`. ### Notes * This PR depends on #12509: before it, the GC bridge used a `std::unordered_map`, which drags in `__next_prime` and `__libcpp_verbose_abort` from libc++ internals. The rest of this stack is already rebased on top of it, so nothing further is needed. * This only affects **NativeAOT**. The Mono and CoreCLR runtimes still link libc++, and `NativeRuntimeComponents.cs` (the unified-runtime archive list) is deliberately untouched. * ARM64 and x64 do not need the NDK unwind archive. On 32-bit ARM, the application resolves EHABI personalities from the NativeAOT runtime's existing private unwinder after symbol promotion. ### Testing Built, installed and launched a default MAUI app on an API 36 arm64 emulator. Cold start with no crashes. `llvm-nm --undefined-only` on the resulting `libnaot-android.release-static-release.a` reports no `operator new`/`operator delete`, `__cxa_*`, `_Unwind_*` or `__libcpp_*` references. The only remaining `std::` symbols are `string_view` appearing in mangled names, which is header-only and carries no runtime dependency. The 32-bit ARM path is covered locally in both linker configurations: * `BuildNativeAot_AndroidArm_WithoutNdk` — workload linker. * `BuildNativeAot_AndroidArm_WithNdkLinker` — NDK linker. Both tests build successfully and assert that the linker response uses `libRuntime.WorkstationGC.arm-ehabi.a` and does not contain `libunwind.a`. --- ## Also: stop shipping the archives in the runtime packs Previously a separate PR stacked directly on this one; folded in here because "stop linking it" and "stop shipping it" are the same change to the reader, and reviewing them apart means reading the same targets twice. The NativeAOT runtime packs still shipped `libc++_static.a`, `libc++abi.a` and `libunwind.a` even though, after the change above, nothing links them any more. ### Why this needs a new item kind `_AndroidNdkRedistributable` (in `build-tools/scripts/Ndk.targets`) tagged NDK files with just two kinds: * `System` — `libc.so`, `libdl.so`, `liblog.so`, `libm.so`, `libz.so` — shipped to every runtime. * `Toolchain` — `crtbegin_so.o`, `crtend_so.o`, `libc++_static.a`, `libc++abi.a`, `libclang_rt.builtins-*.a`, `libunwind.a` — shipped to CoreCLR and NativeAOT, since both do native linking. NativeAOT still needs `crtbegin_so.o`, `crtend_so.o` and `libclang_rt.builtins-*.a`, so the `Toolchain` group cannot just be dropped for NativeAOT. This adds a third kind, `CplusPlus`, for the three C++ archives, and ships it only for CoreCLR. Both packaging sites are updated: * `src/native/native.targets` — the local `bin/<Config>/lib/packs` layout. * `build-tools/create-packs/Microsoft.Android.Runtime.proj` — the shipped NuGet packs. ### Size Per ABI, removed from the NativeAOT runtime pack: | Archive | Size | | --- | ---: | | `libc++_static.a` | 15,182,348 | | `libc++abi.a` | 3,125,348 | | `libunwind.a` | 91,152 | | **Total** | **18,398,848** | Across `android-arm`, `android-arm64` and `android-x64` that is roughly **55 MB** of pack content. This does not change application size — that is the linker change above — but it shrinks what users restore. ### Testing Deleted each pack directory and regenerated it via `_CopyToPackDirs`, rather than checking a pack that could still contain stale files. NativeAOT (`android-arm64`) — the three archives are gone, and everything NativeAOT links is still present: ``` crtbegin_so.o crtend_so.o libc.so libclang_rt.builtins-aarch64-android.a libdl.so liblog.so libm.so libz.so libnaot-android.debug-static-debug.a libnaot-android.debug.so libnaot-android.release-static-release.a libnaot-android.release.so libxa-java-interop-release.a ``` CoreCLR (`android-arm64`) — all three are still shipped: ``` crtbegin_so.o crtend_so.o libarchive-dso-stub.so libc.so libc++_static.a libc++abi.a libclang_rt.builtins-aarch64-android.a libdl.so liblog.so libm.so libunwind.a libz.so ``` Mono is unaffected — it only ever received the `System` kind.
Summary
Remove simple CLR local strings while preserving dynamic growth where the old local-string type supported it.
Changes
malloc()storage(buffer, buffer_size)calls using the platform property boundmonodroid_get_system_property(..., dynamic_local_property_string&)overloadValidation
Follow-up: return
const char*rather thanstd::string_viewThe first pass here replaced the local string with a fixed caller-supplied buffer, which regressed bundled properties: those come from
@(AndroidEnvironment)and are not bound by Android'sPROP_VALUE_MAX, so any value longer than the buffer was reported as absent. Returning a view fixed that — Android system properties view the caller's scratch buffer, bundled properties point straight at immortal static application data, with no copy and no length cap.That part stands, but
std::string_viewwas the wrong vehicle for it. Every value the function can return is NUL-terminated (__system_property_get()terminates what it writes, and bundled properties are NUL-terminated static strings), yetstring_viewexplicitly promises the opposite. The invariant had to be asserted in a header comment — "The returned value is always NUL-terminated" — precisely because the type denied it, andget_max_gref_count_from_system()quietly depended on it by handing.data()tostrtol()and to a%sconversion.So the getter now returns
const char*(nullptrwhen the property is unset), in both the CoreCLR and MonoVM implementations:.data()no longer has to be laundered through astring_viewto reach a C API;Logger::init_logging_categories(),FastTiming::parse_options()) construct astd::string_viewexplicitly, which is honest about what they are doing.No change in libc++ references (
string_viewis header-only) — this is a type-safety fix, not a size one. All three runtime lanes build clean.Follow-up: fixes from the offline review
Reviewing the conversion above turned up three problems in the code it touches, all fixed in the last commit.
lookup_system_property()returned the property name, not its value. It returnedprop_iter->first— the map key — while reportingprop_iter->second.length()as the length. In a DEBUG build with bundled@(AndroidEnvironment)properties,Logger::init_logging_categories()andget_max_gref_count_from_system()were parsing the property name. This predates the PR, but the conversion above makes that value the direct return of the public getter, and the no-copy contract documented in the header describes exactly this path, so it belongs here.monodroid__system_property_get()wrote one byte past the end of the caller's buffer. Its small-buffer fallback copied through a heap buffer and then didsp_value[sp_value_len] = '\0'. The branch was already dead — the only caller rejects undersized buffers before calling — so thesp_value_lenparameter is gone along with it. That also drops anew[]/delete[]pair, removing two libc++ references. #12523 previously carried its own version of this removal; it is now fixed here, in the PR that makes the path unreachable.An undersized buffer was reported as an unset property. Returning
nullptrconflated a programming error with a legitimate runtime outcome, so it is anabort_unless()now.Also derives the
"gref="/"lref="prefix length withsizeof()instead of hardcoding5.All three runtime lanes build clean; libc++ references at the stack tip are unchanged at 21.