From 64f53d18b339f25307aebdf2a6761889ff142163 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 15:51:10 +0200 Subject: [PATCH 01/26] Replace simple CLR local strings Use fixed buffers for straightforward type-name, override-path, and system-property values, formatting composed strings with snprintf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/assembly-store.cc | 8 +-- src/native/clr/host/typemap.cc | 15 +++-- .../include/runtime-base/android-system.hh | 64 +++++++------------ .../clr/runtime-base/android-system-shared.cc | 31 +++++++++ 4 files changed, 67 insertions(+), 51 deletions(-) diff --git a/src/native/clr/host/assembly-store.cc b/src/native/clr/host/assembly-store.cc index 12ef3884d57..1e77617fa41 100644 --- a/src/native/clr/host/assembly-store.cc +++ b/src/native/clr/host/assembly-store.cc @@ -296,11 +296,11 @@ namespace { return; } { - dynamic_local_property_string prop_value; - if (AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value) > 0 && prop_value.get () != nullptr) { - if (prop_value.get ()[0] == '0') { + char prop_value[Constants::PROPERTY_VALUE_BUFFER_LEN]; + if (AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)) > 0) { + if (prop_value [0] == '0') { cache_requested = false; - } else if (prop_value.get ()[0] == '1') { + } else if (prop_value [0] == '1') { cache_requested = true; } } diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 71748a13410..0679948c582 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -152,14 +153,16 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap [[gnu::always_inline, gnu::flatten]] auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { - dynamic_local_path_string full_type_name; - full_type_name.append (typeName); - full_type_name.append (", "sv); - full_type_name.append (assemblyFullName); + char full_type_name[Constants::SENSIBLE_PATH_MAX]; + int length = snprintf (full_type_name, sizeof (full_type_name), "%s, %s", typeName, assemblyFullName); + if (length < 0 || static_cast(length) >= sizeof (full_type_name)) [[unlikely]] { + log_warn (LOG_ASSEMBLY, "typemap: managed type name is too long"); + return nullptr; + } - ssize_t idx = find_index_by_hash (full_type_name.get (), type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); + ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); - return index_to_name (idx, full_type_name.get (), type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); + return index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); } #endif // def DEBUG diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 8a13669e9cc..c42b2cace3c 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -1,7 +1,7 @@ #pragma once #include -#include +#include #include #include #include @@ -79,7 +79,7 @@ namespace xamarin::android { static void set_primary_override_dir (jstring_wrapper& home) noexcept { #if defined (XA_HOST_NATIVEAOT) - determine_primary_override_dir (home, primary_override_dir, sizeof (primary_override_dir)); + format_primary_override_dir (home, primary_override_dir, sizeof (primary_override_dir)); #else primary_override_dir = determine_primary_override_dir (home); #endif @@ -111,8 +111,8 @@ namespace xamarin::android { * However, if any logging is enabled (which should _not_ happen with * pre-loaded apps!), we need the .__override__ directory... */ - dynamic_local_property_string value; - if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value) == 0) [[likely]] { + char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; + if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) == 0) [[likely]] { return; } } @@ -128,25 +128,7 @@ namespace xamarin::android { } static auto monodroid_get_system_property (std::string_view const& name, dynamic_local_property_string &value) noexcept -> int; - - template - static auto monodroid_get_system_property (std::string_view const& name, char (&value)[Size]) noexcept -> int - { - dynamic_local_property_string property_value; - int result = monodroid_get_system_property (name, property_value); - if (result > 0) { - if (property_value.length () >= Size) { - value [0] = '\0'; - return -1; - } - memcpy (value, property_value.get (), property_value.length ()); - value [property_value.length ()] = '\0'; - } else { - value [0] = '\0'; - } - return result; - } - + static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> int; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; static void setup_app_library_directories (jstring_array_wrapper& runtimeApks, jstring_array_wrapper& appDirs, bool have_split_apks) noexcept; @@ -175,28 +157,28 @@ namespace xamarin::android { embedded_dso_mode_enabled = yesno; } -#if defined (XA_HOST_NATIVEAOT) - static void determine_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept + static auto format_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept -> size_t { - dynamic_local_string name { home.get_cstr () }; - name.append ("/") - .append (Constants::OVERRIDE_DIRECTORY_NAME) - .append ("/") - .append (Constants::android_lib_abi); - - abort_unless (name.length () < buffer_size, "Primary override directory path is too long"); - memcpy (buffer, name.get (), name.length () + 1); + int length = snprintf ( + buffer, + buffer_size, + "%s/%.*s/%.*s", + home.get_cstr (), + static_cast(Constants::OVERRIDE_DIRECTORY_NAME.length ()), + Constants::OVERRIDE_DIRECTORY_NAME.data (), + static_cast(Constants::android_lib_abi.length ()), + Constants::android_lib_abi.data () + ); + abort_unless (length >= 0 && static_cast(length) < buffer_size, "Primary override directory path is too long"); + return static_cast(length); } -#else + +#if !defined (XA_HOST_NATIVEAOT) static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { - dynamic_local_string name { home.get_cstr () }; - name.append ("/") - .append (Constants::OVERRIDE_DIRECTORY_NAME) - .append ("/") - .append (Constants::android_lib_abi); - - return {name.get (), name.length ()}; + char name[Constants::SENSIBLE_PATH_MAX]; + size_t length = format_primary_override_dir (home, name, sizeof (name)); + return {name, length}; } #endif diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index 7eeaa4fc7c6..3d7054c9151 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -7,6 +7,37 @@ using namespace xamarin::android; using std::operator""sv; +auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> int +{ + if (value == nullptr || value_size == 0) { + return -1; + } + + value [0] = '\0'; + if (value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { + return -1; + } + + int len = monodroid__system_property_get (name, value, value_size); + if (len > 0) { + return len; + } + + size_t property_length; + const char *property_value = lookup_system_property (name, property_length); + if (property_value == nullptr) { + return len; + } + + if (property_length >= value_size) { + return -1; + } + + memcpy (value, property_value, property_length); + value [property_length] = '\0'; + return Helpers::add_with_overflow_check (property_length, 0); +} + auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, dynamic_local_property_string &value) noexcept -> int { int len = monodroid__system_property_get (name, value.get (), value.size ()); From fa4f4f9ebdc0daa1e82e5cd3ab295c7c6695f676 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 16:13:36 +0200 Subject: [PATCH 02/26] Remove CLR dynamic property strings Move the remaining logger, max-gref, and timing property consumers to explicit fixed buffers so the CLR dynamic-local-string property overload can be removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../include/runtime-base/android-system.hh | 3 +- src/native/clr/include/runtime-base/logger.hh | 2 +- .../clr/runtime-base/android-system-shared.cc | 27 +++------------ src/native/clr/runtime-base/logger.cc | 34 +++++++++++-------- .../common/runtime-base/timing-internal.cc | 5 ++- .../mono/runtime-base/android-system.cc | 32 +++++++++++++++++ .../mono/runtime-base/android-system.hh | 1 + 7 files changed, 61 insertions(+), 43 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index c42b2cace3c..0266ed79d03 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -112,7 +112,7 @@ namespace xamarin::android { * pre-loaded apps!), we need the .__override__ directory... */ char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) == 0) [[likely]] { + if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) <= 0) [[likely]] { return; } } @@ -127,7 +127,6 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - static auto monodroid_get_system_property (std::string_view const& name, dynamic_local_property_string &value) noexcept -> int; static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> int; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; diff --git a/src/native/clr/include/runtime-base/logger.hh b/src/native/clr/include/runtime-base/logger.hh index 0c6121a6bde..9937a4d1a9f 100644 --- a/src/native/clr/include/runtime-base/logger.hh +++ b/src/native/clr/include/runtime-base/logger.hh @@ -54,7 +54,7 @@ namespace xamarin::android { static auto open_file (LogCategories category, std::string_view const& custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE*; private: - static bool set_category (std::string_view const& name, string_segment& arg, unsigned int entry, bool arg_starts_with_name = false) noexcept; + static bool set_category (std::string_view const& name, std::string_view const& arg, unsigned int entry, bool arg_starts_with_name = false) noexcept; private: static inline LogTimingCategories _log_timing_categories; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index 3d7054c9151..87a4330ac83 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -38,25 +38,6 @@ auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, return Helpers::add_with_overflow_check (property_length, 0); } -auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, dynamic_local_property_string &value) noexcept -> int -{ - int len = monodroid__system_property_get (name, value.get (), value.size ()); - if (len > 0) { - // Clumsy, but if we want direct writes to be fast, this is the price we pay - value.set_length_after_direct_write (static_cast(len)); - return len; - } - - size_t plen; - const char *v = lookup_system_property (name, plen); - if (v == nullptr) { - return len; - } - - value.assign (v, plen); - return Helpers::add_with_overflow_check (plen, 0); -} - auto AndroidSystem::monodroid__system_property_get (std::string_view const& name, char *sp_value, size_t sp_value_len) noexcept -> int { @@ -92,10 +73,10 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long max = 51200; } - dynamic_local_property_string override; - if (monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override) > 0) { + char override[Constants::PROPERTY_VALUE_BUFFER_LEN]; + if (monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)) > 0) { char *e; - max = strtol (override.get (), &e, 10); + max = strtol (override, &e, 10); switch (*e) { case 'k': e++; @@ -117,7 +98,7 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long "Unsupported '%.*s' value '%s'.", static_cast(Constants::DEBUG_MONO_MAX_GREFC.length ()), Constants::DEBUG_MONO_MAX_GREFC.data (), - override.get () + override ); } diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index c00d23e6cad..9ed0d81c9c3 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -120,13 +120,13 @@ Logger::init_reference_logging (std::string_view const& override_dir) noexcept } [[gnu::always_inline]] bool -Logger::set_category (std::string_view const& name, string_segment& arg, unsigned int entry, bool arg_starts_with_name) noexcept +Logger::set_category (std::string_view const& name, std::string_view const& arg, unsigned int entry, bool arg_starts_with_name) noexcept { if ((log_categories & entry) == entry) { return false; } - if (arg_starts_with_name ? arg.starts_with (name) : arg.equal (name)) { + if (arg_starts_with_name ? arg.starts_with (name) : arg == name) { log_categories |= entry; return true; } @@ -139,16 +139,25 @@ Logger::init_logging_categories () noexcept { _log_timing_categories = LogTimingCategories::Default; - dynamic_local_property_string value; - if (AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value) == 0) { + char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; + int value_length = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); + if (value_length <= 0) { return; } - string_segment param; - while (value.next_token (',', param)) { + std::string_view remaining { value, static_cast(value_length) }; + while (!remaining.empty ()) { + size_t separator = remaining.find (','); + std::string_view param = remaining.substr (0, separator); + if (separator == std::string_view::npos) { + remaining = {}; + } else { + remaining.remove_prefix (separator + 1); + } + constexpr std::string_view CAT_ALL { "all" }; - if (param.equal (CAT_ALL)) { + if (param == CAT_ALL) { log_categories = 0xFFFFFFFF; break; } @@ -189,21 +198,19 @@ Logger::init_logging_categories () noexcept continue; } - auto get_log_file_name = [](std::string_view const& file_kind, string_segment const& segment, size_t offset) -> std::string_view { - auto file_name = segment.at (offset); - - if (!file_name.has_value ()) { + auto get_log_file_name = [](std::string_view const& file_kind, std::string_view const& segment, size_t offset) -> std::string_view { + if (offset >= segment.length ()) { log_warnf ( LOG_DEFAULT, "Unable to set path to %.*s log file: %s", static_cast(file_kind.length ()), file_kind.data (), - to_string (file_name.error ()) + "no file name specified" ); return {}; } - return { file_name.value (), segment.length () - offset }; + return segment.substr (offset); }; constexpr std::string_view CAT_GREF_EQUALS { "gref=" }; @@ -247,7 +254,6 @@ Logger::init_logging_categories () noexcept if (param.starts_with ("timing=bare")) { log_categories |= LOG_TIMING; _log_timing_categories |= LogTimingCategories::Bare; - continue; } } diff --git a/src/native/common/runtime-base/timing-internal.cc b/src/native/common/runtime-base/timing-internal.cc index e399e1ed17a..638c0732a6b 100644 --- a/src/native/common/runtime-base/timing-internal.cc +++ b/src/native/common/runtime-base/timing-internal.cc @@ -30,8 +30,7 @@ void FastTiming::really_initialize (bool log_immediately) noexcept } char value [Constants::PROPERTY_VALUE_BUFFER_LEN]; - int value_length = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value); - if (value_length > 0) { + if (AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)) > 0) { internal_timing.parse_options (value); } @@ -61,7 +60,7 @@ void FastTiming::parse_options (char *value) noexcept errno = 0; unsigned long long parsed_duration = strtoull (duration, &end, 10); if (end == duration || *end != '\0' || errno == ERANGE || parsed_duration > std::numeric_limits::max ()) { - log_warn (LOG_TIMING, "Failed to parse duration in milliseconds from '%s'"sv, param); + log_warn (LOG_TIMING, "Failed to parse duration in milliseconds from '{}'"sv, param); duration_ms = default_duration_milliseconds; } else { duration_ms = static_cast(parsed_duration); diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 94a5d39a63d..85a0d63f4a7 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -172,6 +172,38 @@ AndroidSystem::monodroid_get_system_property (const char *name, dynamic_local_st return Helpers::add_with_overflow_check (plen, 0); } +int +AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept +{ + if (value == nullptr || value_size == 0) { + return -1; + } + + value [0] = '\0'; + if (value_size < PROPERTY_VALUE_BUFFER_LEN) { + return -1; + } + + int len = _monodroid__system_property_get (name.data (), value, value_size); + if (len > 0) { + return len; + } + + size_t property_length; + const char *property_value = lookup_system_property (name.data (), property_length); + if (property_value == nullptr) { + return len; + } + + if (property_length >= value_size) { + return -1; + } + + memcpy (value, property_value, property_length); + value [property_length] = '\0'; + return Helpers::add_with_overflow_check (property_length, 0); +} + int AndroidSystem::monodroid_get_system_property (const char *name, char **value) noexcept { diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index 1a39e3c7cf0..a38c0ac472e 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -95,6 +95,7 @@ namespace xamarin::android::internal { static void create_update_dir (char *override_dir) noexcept; static int monodroid_get_system_property (const char *name, char **value) noexcept; static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; + static int monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; static int monodroid_get_system_property (std::string_view const& name, char **value) noexcept { From 72b86d476ffc9fbff5246ddb2404fc67d3bc73e1 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 16:55:24 +0200 Subject: [PATCH 03/26] Narrow the logger strings include The logger interface no longer exposes local-string types, so keep the temporary include local to its remaining fallback-path implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/include/runtime-base/logger.hh | 1 - src/native/clr/runtime-base/logger.cc | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/native/clr/include/runtime-base/logger.hh b/src/native/clr/include/runtime-base/logger.hh index 9937a4d1a9f..9bf67148fc1 100644 --- a/src/native/clr/include/runtime-base/logger.hh +++ b/src/native/clr/include/runtime-base/logger.hh @@ -5,7 +5,6 @@ #include #include -#include namespace xamarin::android { class Logger diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 9ed0d81c9c3..04a5a6f83b8 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include From 962c572f8a82c06ffe67703de2a345fec14fa636 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 17:22:53 +0200 Subject: [PATCH 04/26] Preserve unbounded generated names Allocate managed type and timing strings to their exact sizes instead of treating the former local-string stack threshold as a maximum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 0679948c582..f8a999df686 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -153,16 +154,21 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap [[gnu::always_inline, gnu::flatten]] auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { - char full_type_name[Constants::SENSIBLE_PATH_MAX]; - int length = snprintf (full_type_name, sizeof (full_type_name), "%s, %s", typeName, assemblyFullName); - if (length < 0 || static_cast(length) >= sizeof (full_type_name)) [[unlikely]] { - log_warn (LOG_ASSEMBLY, "typemap: managed type name is too long"); - return nullptr; - } + size_t type_name_length = strlen (typeName); + size_t assembly_name_length = strlen (assemblyFullName); + size_t full_type_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); + full_type_name_length = Helpers::add_with_overflow_check (full_type_name_length, 2uz); + size_t allocation_size = Helpers::add_with_overflow_check (full_type_name_length, 1uz); + auto full_type_name = std::make_unique (allocation_size); + + memcpy (full_type_name.get (), typeName, type_name_length); + memcpy (full_type_name.get () + type_name_length, ", ", 2uz); + memcpy (full_type_name.get () + type_name_length + 2uz, assemblyFullName, assembly_name_length); + full_type_name [full_type_name_length] = '\0'; - ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); + ssize_t idx = find_index_by_hash (full_type_name.get (), type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); - return index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); + return index_to_name (idx, full_type_name.get (), type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); } #endif // def DEBUG From 23ac7c98c10e1f54e1c1e4df830afbc92df6d560 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 17:30:57 +0200 Subject: [PATCH 05/26] Use C allocation for generated names Keep exact-size type and timing strings independent of libc++ ownership. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index f8a999df686..f19de26a23c 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -1,8 +1,8 @@ #include #include +#include #include #include -#include #include #include @@ -159,16 +159,19 @@ auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemb size_t full_type_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); full_type_name_length = Helpers::add_with_overflow_check (full_type_name_length, 2uz); size_t allocation_size = Helpers::add_with_overflow_check (full_type_name_length, 1uz); - auto full_type_name = std::make_unique (allocation_size); + auto full_type_name = static_cast (std::malloc (allocation_size)); + abort_unless (full_type_name != nullptr, "Failed to allocate managed type name"); - memcpy (full_type_name.get (), typeName, type_name_length); - memcpy (full_type_name.get () + type_name_length, ", ", 2uz); - memcpy (full_type_name.get () + type_name_length + 2uz, assemblyFullName, assembly_name_length); + memcpy (full_type_name, typeName, type_name_length); + memcpy (full_type_name + type_name_length, ", ", 2uz); + memcpy (full_type_name + type_name_length + 2uz, assemblyFullName, assembly_name_length); full_type_name [full_type_name_length] = '\0'; - ssize_t idx = find_index_by_hash (full_type_name.get (), type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); + ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); + const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); + std::free (full_type_name); - return index_to_name (idx, full_type_name.get (), type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); + return mapped_name; } #endif // def DEBUG From 48a37eba9306a10004fdfaaba1e2e6d6400c2b93 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 17:35:43 +0200 Subject: [PATCH 06/26] Allocate override paths to their exact size Keep the existing NativeAOT fixed-storage limit while preserving unbounded CoreCLR path construction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../include/runtime-base/android-system.hh | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 0266ed79d03..8105644848f 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -175,9 +176,27 @@ namespace xamarin::android { #if !defined (XA_HOST_NATIVEAOT) static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { - char name[Constants::SENSIBLE_PATH_MAX]; - size_t length = format_primary_override_dir (home, name, sizeof (name)); - return {name, length}; + std::string_view home_path = home.get_string_view (); + size_t length = Helpers::add_with_overflow_check (home_path.length (), Constants::OVERRIDE_DIRECTORY_NAME.length ()); + length = Helpers::add_with_overflow_check (length, Constants::android_lib_abi.length ()); + length = Helpers::add_with_overflow_check (length, 2uz); + size_t allocation_size = Helpers::add_with_overflow_check (length, 1uz); + auto name = static_cast (std::malloc (allocation_size)); + abort_unless (name != nullptr, "Failed to allocate primary override directory path"); + + char *destination = name; + memcpy (destination, home_path.data (), home_path.length ()); + destination += home_path.length (); + *destination++ = Constants::DIR_SEP [0]; + memcpy (destination, Constants::OVERRIDE_DIRECTORY_NAME.data (), Constants::OVERRIDE_DIRECTORY_NAME.length ()); + destination += Constants::OVERRIDE_DIRECTORY_NAME.length (); + *destination++ = Constants::DIR_SEP [0]; + memcpy (destination, Constants::android_lib_abi.data (), Constants::android_lib_abi.length ()); + name [length] = '\0'; + + std::string path { name, length }; + std::free (name); + return path; } #endif From 56b79d1fa84a2bae2f102dd7e811ac67cf0100d7 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 17:50:55 +0200 Subject: [PATCH 07/26] Keep short generated names on the stack Use malloc only when typemap or override names exceed their sensible local buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 7 +++---- src/native/clr/include/runtime-base/android-system.hh | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index f19de26a23c..833c128e1fb 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -1,6 +1,5 @@ #include #include -#include #include #include @@ -159,8 +158,8 @@ auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemb size_t full_type_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); full_type_name_length = Helpers::add_with_overflow_check (full_type_name_length, 2uz); size_t allocation_size = Helpers::add_with_overflow_check (full_type_name_length, 1uz); - auto full_type_name = static_cast (std::malloc (allocation_size)); - abort_unless (full_type_name != nullptr, "Failed to allocate managed type name"); + char local_buffer [Constants::SENSIBLE_PATH_MAX]; + char *full_type_name = Helpers::get_temporary_buffer (local_buffer, allocation_size); memcpy (full_type_name, typeName, type_name_length); memcpy (full_type_name + type_name_length, ", ", 2uz); @@ -169,7 +168,7 @@ auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemb ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); - std::free (full_type_name); + Helpers::free_temporary_buffer (full_type_name, local_buffer); return mapped_name; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 8105644848f..ba5d4ddf86e 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -181,8 +180,8 @@ namespace xamarin::android { length = Helpers::add_with_overflow_check (length, Constants::android_lib_abi.length ()); length = Helpers::add_with_overflow_check (length, 2uz); size_t allocation_size = Helpers::add_with_overflow_check (length, 1uz); - auto name = static_cast (std::malloc (allocation_size)); - abort_unless (name != nullptr, "Failed to allocate primary override directory path"); + char local_buffer [Constants::SENSIBLE_PATH_MAX]; + char *name = Helpers::get_temporary_buffer (local_buffer, allocation_size); char *destination = name; memcpy (destination, home_path.data (), home_path.length ()); @@ -195,7 +194,7 @@ namespace xamarin::android { name [length] = '\0'; std::string path { name, length }; - std::free (name); + Helpers::free_temporary_buffer (name, local_buffer); return path; } #endif From 931f2930fd61c4e47da5b62a022b5a72b6d461d9 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 20:47:06 +0200 Subject: [PATCH 08/26] Retry formatting oversized CLR strings Preserve stack storage for typical managed type and override paths while allocating the exact required capacity for larger values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 45 +++++++++++++----- .../include/runtime-base/android-system.hh | 47 ++++++++++--------- 2 files changed, 57 insertions(+), 35 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 833c128e1fb..4b77fbf7412 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -1,7 +1,9 @@ #include #include +#include #include #include +#include #include #include @@ -19,6 +21,25 @@ namespace { return value_length == key_length && strncmp (value, key, key_length) == 0; } + auto format_managed_type_name (const char *type_name, const char *assembly_name, char *buffer, size_t buffer_size) noexcept -> ssize_t + { + size_t type_name_length = strlen (type_name); + size_t assembly_name_length = strlen (assembly_name); + size_t full_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); + full_name_length = Helpers::add_with_overflow_check (full_name_length, 2uz); + size_t required_capacity = Helpers::add_with_overflow_check (full_name_length, 1uz); + abort_unless (required_capacity <= static_cast(std::numeric_limits::max ()), "Managed type name is too long"); + if (buffer == nullptr || buffer_size < required_capacity) { + return -static_cast(required_capacity); + } + + memcpy (buffer, type_name, type_name_length); + memcpy (buffer + type_name_length, ", ", 2uz); + memcpy (buffer + type_name_length + 2uz, assembly_name, assembly_name_length); + buffer [full_name_length] = '\0'; + return static_cast(full_name_length); + } + class MonoGuidString { static inline constexpr size_t MVID_SIZE = 16; @@ -153,22 +174,22 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap [[gnu::always_inline, gnu::flatten]] auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { - size_t type_name_length = strlen (typeName); - size_t assembly_name_length = strlen (assemblyFullName); - size_t full_type_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); - full_type_name_length = Helpers::add_with_overflow_check (full_type_name_length, 2uz); - size_t allocation_size = Helpers::add_with_overflow_check (full_type_name_length, 1uz); char local_buffer [Constants::SENSIBLE_PATH_MAX]; - char *full_type_name = Helpers::get_temporary_buffer (local_buffer, allocation_size); - - memcpy (full_type_name, typeName, type_name_length); - memcpy (full_type_name + type_name_length, ", ", 2uz); - memcpy (full_type_name + type_name_length + 2uz, assemblyFullName, assembly_name_length); - full_type_name [full_type_name_length] = '\0'; + char *heap_buffer = nullptr; + char *full_type_name = local_buffer; + ssize_t result = format_managed_type_name (typeName, assemblyFullName, full_type_name, sizeof (local_buffer)); + if (result < 0) { + size_t required_capacity = static_cast(-result); + heap_buffer = static_cast (std::malloc (required_capacity)); + abort_unless (heap_buffer != nullptr, "Failed to allocate managed type name"); + full_type_name = heap_buffer; + result = format_managed_type_name (typeName, assemblyFullName, full_type_name, required_capacity); + } + abort_unless (result >= 0, "Failed to format managed type name using the required capacity"); ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); - Helpers::free_temporary_buffer (full_type_name, local_buffer); + std::free (heap_buffer); return mapped_name; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index ba5d4ddf86e..d07919a3c55 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -79,7 +79,8 @@ namespace xamarin::android { static void set_primary_override_dir (jstring_wrapper& home) noexcept { #if defined (XA_HOST_NATIVEAOT) - format_primary_override_dir (home, primary_override_dir, sizeof (primary_override_dir)); + ssize_t result = format_primary_override_dir (home, primary_override_dir, sizeof (primary_override_dir)); + abort_unless (result >= 0, "Primary override directory path is too long"); #else primary_override_dir = determine_primary_override_dir (home); #endif @@ -156,7 +157,7 @@ namespace xamarin::android { embedded_dso_mode_enabled = yesno; } - static auto format_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept -> size_t + static auto format_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept -> ssize_t { int length = snprintf ( buffer, @@ -168,33 +169,33 @@ namespace xamarin::android { static_cast(Constants::android_lib_abi.length ()), Constants::android_lib_abi.data () ); - abort_unless (length >= 0 && static_cast(length) < buffer_size, "Primary override directory path is too long"); - return static_cast(length); + abort_unless (length >= 0, "Failed to format primary override directory path"); + size_t required_capacity = Helpers::add_with_overflow_check (static_cast(length), 1uz); + abort_unless (required_capacity <= static_cast(std::numeric_limits::max ()), "Primary override directory path is too long"); + if (buffer == nullptr || buffer_size < required_capacity) { + return -static_cast(required_capacity); + } + return static_cast(length); } #if !defined (XA_HOST_NATIVEAOT) static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { - std::string_view home_path = home.get_string_view (); - size_t length = Helpers::add_with_overflow_check (home_path.length (), Constants::OVERRIDE_DIRECTORY_NAME.length ()); - length = Helpers::add_with_overflow_check (length, Constants::android_lib_abi.length ()); - length = Helpers::add_with_overflow_check (length, 2uz); - size_t allocation_size = Helpers::add_with_overflow_check (length, 1uz); char local_buffer [Constants::SENSIBLE_PATH_MAX]; - char *name = Helpers::get_temporary_buffer (local_buffer, allocation_size); - - char *destination = name; - memcpy (destination, home_path.data (), home_path.length ()); - destination += home_path.length (); - *destination++ = Constants::DIR_SEP [0]; - memcpy (destination, Constants::OVERRIDE_DIRECTORY_NAME.data (), Constants::OVERRIDE_DIRECTORY_NAME.length ()); - destination += Constants::OVERRIDE_DIRECTORY_NAME.length (); - *destination++ = Constants::DIR_SEP [0]; - memcpy (destination, Constants::android_lib_abi.data (), Constants::android_lib_abi.length ()); - name [length] = '\0'; - - std::string path { name, length }; - Helpers::free_temporary_buffer (name, local_buffer); + char *heap_buffer = nullptr; + char *name = local_buffer; + ssize_t result = format_primary_override_dir (home, name, sizeof (local_buffer)); + if (result < 0) { + size_t required_capacity = static_cast(-result); + heap_buffer = static_cast (std::malloc (required_capacity)); + abort_unless (heap_buffer != nullptr, "Failed to allocate primary override directory path"); + name = heap_buffer; + result = format_primary_override_dir (home, name, required_capacity); + } + abort_unless (result >= 0, "Failed to format primary override directory path using the required capacity"); + + std::string path { name, static_cast(result) }; + std::free (heap_buffer); return path; } #endif From 21a8325fcc899d716453d0d8a0deb5903f0f2b11 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 20:48:17 +0200 Subject: [PATCH 09/26] Use shared CLR buffer cleanup Route managed type and override path heap-buffer cleanup through Util. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 2 +- src/native/clr/include/runtime-base/android-system.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 4b77fbf7412..46d2b0fc0c2 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -189,7 +189,7 @@ auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemb ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); - std::free (heap_buffer); + Util::free_if_used (heap_buffer); return mapped_name; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index d07919a3c55..64677864895 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -195,7 +195,7 @@ namespace xamarin::android { abort_unless (result >= 0, "Failed to format primary override directory path using the required capacity"); std::string path { name, static_cast(result) }; - std::free (heap_buffer); + Util::free_if_used (heap_buffer); return path; } #endif From 833f4aab237f3ea39553b81758cfee859d74e2b7 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 23:23:59 +0200 Subject: [PATCH 10/26] Use free directly for CLR buffers Rely on free(nullptr) and name stack-backed CLR string storage explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 8 ++++---- src/native/clr/include/runtime-base/android-system.hh | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 46d2b0fc0c2..031fb836892 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -174,10 +174,10 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap [[gnu::always_inline, gnu::flatten]] auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { - char local_buffer [Constants::SENSIBLE_PATH_MAX]; + char stack_buffer [Constants::SENSIBLE_PATH_MAX]; char *heap_buffer = nullptr; - char *full_type_name = local_buffer; - ssize_t result = format_managed_type_name (typeName, assemblyFullName, full_type_name, sizeof (local_buffer)); + char *full_type_name = stack_buffer; + ssize_t result = format_managed_type_name (typeName, assemblyFullName, full_type_name, sizeof (stack_buffer)); if (result < 0) { size_t required_capacity = static_cast(-result); heap_buffer = static_cast (std::malloc (required_capacity)); @@ -189,7 +189,7 @@ auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemb ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); - Util::free_if_used (heap_buffer); + std::free (heap_buffer); return mapped_name; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 64677864895..c388476b04d 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -181,10 +181,10 @@ namespace xamarin::android { #if !defined (XA_HOST_NATIVEAOT) static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { - char local_buffer [Constants::SENSIBLE_PATH_MAX]; + char stack_buffer [Constants::SENSIBLE_PATH_MAX]; char *heap_buffer = nullptr; - char *name = local_buffer; - ssize_t result = format_primary_override_dir (home, name, sizeof (local_buffer)); + char *name = stack_buffer; + ssize_t result = format_primary_override_dir (home, name, sizeof (stack_buffer)); if (result < 0) { size_t required_capacity = static_cast(-result); heap_buffer = static_cast (std::malloc (required_capacity)); @@ -195,7 +195,7 @@ namespace xamarin::android { abort_unless (result >= 0, "Failed to format primary override directory path using the required capacity"); std::string path { name, static_cast(result) }; - Util::free_if_used (heap_buffer); + std::free (heap_buffer); return path; } #endif From 6ee0f1d5a6099ccf733ddf9736b11b6dee625d97 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 25 Aug 2026 23:39:42 +0200 Subject: [PATCH 11/26] Use CLR pointers for buffer ownership Eliminate separate heap pointers and free generated CLR strings only when they differ from their stack buffers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/native/clr/host/typemap.cc | 10 +++++----- src/native/clr/include/runtime-base/android-system.hh | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 031fb836892..552091453d4 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -175,21 +175,21 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { char stack_buffer [Constants::SENSIBLE_PATH_MAX]; - char *heap_buffer = nullptr; char *full_type_name = stack_buffer; ssize_t result = format_managed_type_name (typeName, assemblyFullName, full_type_name, sizeof (stack_buffer)); if (result < 0) { size_t required_capacity = static_cast(-result); - heap_buffer = static_cast (std::malloc (required_capacity)); - abort_unless (heap_buffer != nullptr, "Failed to allocate managed type name"); - full_type_name = heap_buffer; + full_type_name = static_cast (std::malloc (required_capacity)); + abort_unless (full_type_name != nullptr, "Failed to allocate managed type name"); result = format_managed_type_name (typeName, assemblyFullName, full_type_name, required_capacity); } abort_unless (result >= 0, "Failed to format managed type name using the required capacity"); ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); - std::free (heap_buffer); + if (full_type_name != stack_buffer) { + std::free (full_type_name); + } return mapped_name; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index c388476b04d..0b52e20c685 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -182,20 +182,20 @@ namespace xamarin::android { static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { char stack_buffer [Constants::SENSIBLE_PATH_MAX]; - char *heap_buffer = nullptr; char *name = stack_buffer; ssize_t result = format_primary_override_dir (home, name, sizeof (stack_buffer)); if (result < 0) { size_t required_capacity = static_cast(-result); - heap_buffer = static_cast (std::malloc (required_capacity)); - abort_unless (heap_buffer != nullptr, "Failed to allocate primary override directory path"); - name = heap_buffer; + name = static_cast (std::malloc (required_capacity)); + abort_unless (name != nullptr, "Failed to allocate primary override directory path"); result = format_primary_override_dir (home, name, required_capacity); } abort_unless (result >= 0, "Failed to format primary override directory path using the required capacity"); std::string path { name, static_cast(result) }; - std::free (heap_buffer); + if (name != stack_buffer) { + std::free (name); + } return path; } #endif From a7e70fe7ef02362916734b05b87030a4ab464b97 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 26 Aug 2026 15:13:49 +0200 Subject: [PATCH 12/26] Restore includes that strings.hh used to provide Narrowing the `strings.hh` include in `logger.hh` also removed two symbols that headers were picking up transitively through it: * `strings.hh` included `shared/helpers.hh`, which is where `os-bridge.hh` was getting `abort_unless` from. * `strings.hh` included ``, which is where `bridge-processing.cc` was getting `gettid()` from. Include both explicitly at their point of use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e --- src/native/clr/host/bridge-processing.cc | 1 + src/native/clr/include/host/os-bridge.hh | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/native/clr/host/bridge-processing.cc b/src/native/clr/host/bridge-processing.cc index b4daa4b6c6d..92def2312bd 100644 --- a/src/native/clr/host/bridge-processing.cc +++ b/src/native/clr/host/bridge-processing.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include diff --git a/src/native/clr/include/host/os-bridge.hh b/src/native/clr/include/host/os-bridge.hh index 20768eaa897..138933af2ef 100644 --- a/src/native/clr/include/host/os-bridge.hh +++ b/src/native/clr/include/host/os-bridge.hh @@ -6,6 +6,8 @@ #include +#include + #include "../runtime-base/logger.hh" namespace xamarin::android { From 114f986fab76d4f57d329a2d9b1d11f542cd2260 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 27 Aug 2026 18:06:06 +0200 Subject: [PATCH 13/26] Include strings.hh directly in util.cc Narrowing the strings.hh include in logger.hh removed the transitive path that util.cc relied on for dynamic_local_string, breaking the CoreCLR and NativeAOT builds. Include the header where it is used. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e --- src/native/clr/runtime-base/util.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/native/clr/runtime-base/util.cc b/src/native/clr/runtime-base/util.cc index afb034eaba5..e7878539134 100644 --- a/src/native/clr/runtime-base/util.cc +++ b/src/native/clr/runtime-base/util.cc @@ -4,6 +4,7 @@ #include #include +#include #include using namespace xamarin::android; From 72053e06e41e75e85b236bad241074fae5271c76 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 27 Aug 2026 23:30:23 +0200 Subject: [PATCH 14/26] Return a string_view from monodroid_get_system_property 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 --- src/native/clr/host/assembly-store.cc | 7 ++-- .../include/runtime-base/android-system.hh | 10 ++++- .../clr/runtime-base/android-system-shared.cc | 32 +++++++-------- src/native/clr/runtime-base/logger.cc | 5 +-- .../include/runtime-base/timing-internal.hh | 2 +- .../common/runtime-base/timing-internal.cc | 39 +++++++++++-------- .../mono/runtime-base/android-system.cc | 25 +++++------- .../mono/runtime-base/android-system.hh | 8 +++- 8 files changed, 68 insertions(+), 60 deletions(-) diff --git a/src/native/clr/host/assembly-store.cc b/src/native/clr/host/assembly-store.cc index 1e77617fa41..e7c1a4e5d2d 100644 --- a/src/native/clr/host/assembly-store.cc +++ b/src/native/clr/host/assembly-store.cc @@ -297,10 +297,11 @@ namespace { } { char prop_value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)) > 0) { - if (prop_value [0] == '0') { + std::string_view cache_prop = AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)); + if (!cache_prop.empty ()) { + if (cache_prop [0] == '0') { cache_requested = false; - } else if (prop_value [0] == '1') { + } else if (cache_prop [0] == '1') { cache_requested = true; } } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 0b52e20c685..6ac17b722ff 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -113,7 +113,7 @@ namespace xamarin::android { * pre-loaded apps!), we need the .__override__ directory... */ char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) <= 0) [[likely]] { + if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)).empty ()) [[likely]] { return; } } @@ -128,7 +128,13 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> int; + // Returns a view of the property's value, or an empty view if the property is not set. + // + // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, + // used to receive Android system properties. Build-time (bundled) properties are not + // copied into it - the returned view points at static application data instead, so their + // length is not limited by `value_size`. The returned value is always NUL-terminated. + static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> std::string_view; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; static void setup_app_library_directories (jstring_array_wrapper& runtimeApks, jstring_array_wrapper& appDirs, bool have_split_apks) noexcept; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index 87a4330ac83..ecd120eeb35 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -7,35 +7,30 @@ using namespace xamarin::android; using std::operator""sv; -auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> int +auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> std::string_view { - if (value == nullptr || value_size == 0) { - return -1; + if (value == nullptr || value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { + return {}; } value [0] = '\0'; - if (value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { - return -1; - } int len = monodroid__system_property_get (name, value, value_size); if (len > 0) { - return len; + return { value, static_cast(len) }; } size_t property_length; const char *property_value = lookup_system_property (name, property_length); if (property_value == nullptr) { - return len; - } - - if (property_length >= value_size) { - return -1; + return {}; } - memcpy (value, property_value, property_length); - value [property_length] = '\0'; - return Helpers::add_with_overflow_check (property_length, 0); + // Bundled properties are NUL-terminated strings in static application data which live for as + // long as the process does, so we can return them directly instead of copying them into + // `value`. This also means that, unlike Android system properties, their length is not limited + // by `Constants::PROPERTY_VALUE_BUFFER_LEN`. + return { property_value, property_length }; } auto @@ -74,9 +69,10 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long } char override[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)) > 0) { + std::string_view grefc = monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)); + if (!grefc.empty ()) { char *e; - max = strtol (override, &e, 10); + max = strtol (grefc.data (), &e, 10); switch (*e) { case 'k': e++; @@ -98,7 +94,7 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long "Unsupported '%.*s' value '%s'.", static_cast(Constants::DEBUG_MONO_MAX_GREFC.length ()), Constants::DEBUG_MONO_MAX_GREFC.data (), - override + grefc.data () ); } diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 04a5a6f83b8..3633ac7a93a 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -141,12 +141,11 @@ Logger::init_logging_categories () noexcept _log_timing_categories = LogTimingCategories::Default; char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - int value_length = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); - if (value_length <= 0) { + std::string_view remaining = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); + if (remaining.empty ()) { return; } - std::string_view remaining { value, static_cast(value_length) }; while (!remaining.empty ()) { size_t separator = remaining.find (','); std::string_view param = remaining.substr (0, separator); diff --git a/src/native/common/include/runtime-base/timing-internal.hh b/src/native/common/include/runtime-base/timing-internal.hh index 310f8cb6fb4..0f0b64ca800 100644 --- a/src/native/common/include/runtime-base/timing-internal.hh +++ b/src/native/common/include/runtime-base/timing-internal.hh @@ -487,7 +487,7 @@ namespace xamarin::android { } private: - void parse_options (char *value) noexcept; + void parse_options (std::string_view options) noexcept; static void really_initialize (bool log_immediately) noexcept; [[gnu::always_inline]] diff --git a/src/native/common/runtime-base/timing-internal.cc b/src/native/common/runtime-base/timing-internal.cc index 638c0732a6b..44f36f3cf98 100644 --- a/src/native/common/runtime-base/timing-internal.cc +++ b/src/native/common/runtime-base/timing-internal.cc @@ -30,8 +30,9 @@ void FastTiming::really_initialize (bool log_immediately) noexcept } char value [Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)) > 0) { - internal_timing.parse_options (value); + std::string_view options = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)); + if (!options.empty ()) { + internal_timing.parse_options (options); } log_write ( @@ -41,33 +42,37 @@ void FastTiming::really_initialize (bool log_immediately) noexcept ); } -void FastTiming::parse_options (char *value) noexcept +void FastTiming::parse_options (std::string_view options) noexcept { - char *param = value; - while (param != nullptr && *param != '\0') { - char *separator = strchr (param, ','); - if (separator != nullptr) { - *separator = '\0'; + while (!options.empty ()) { + size_t separator = options.find (','); + std::string_view param = options.substr (0, separator); + if (separator == std::string_view::npos) { + options = {}; + } else { + options.remove_prefix (separator + 1); } - if (strcmp (param, OPT_TO_FILE.data ()) == 0) { + if (param == OPT_TO_FILE) { log_to_file = true; - } else if (strncmp (param, OPT_FILE_NAME.data (), OPT_FILE_NAME.length ()) == 0) { - output_file_name = std::make_unique (param + OPT_FILE_NAME.length ()); - } else if (strncmp (param, OPT_DURATION.data (), OPT_DURATION.length ()) == 0) { - const char *duration = param + OPT_DURATION.length (); + } else if (param.starts_with (OPT_FILE_NAME)) { + output_file_name = std::make_unique (param.substr (OPT_FILE_NAME.length ())); + } else if (param.starts_with (OPT_DURATION)) { + std::string_view duration = param.substr (OPT_DURATION.length ()); + // `duration` is not NUL-terminated, but it is always followed by either the separator + // or the terminator of the whole options string, so `strtoull` cannot run past it. + const char *duration_start = duration.data (); + const char *duration_end = duration_start + duration.length (); char *end; errno = 0; - unsigned long long parsed_duration = strtoull (duration, &end, 10); - if (end == duration || *end != '\0' || errno == ERANGE || parsed_duration > std::numeric_limits::max ()) { + unsigned long long parsed_duration = strtoull (duration_start, &end, 10); + if (end != duration_end || duration.empty () || errno == ERANGE || parsed_duration > std::numeric_limits::max ()) { log_warn (LOG_TIMING, "Failed to parse duration in milliseconds from '{}'"sv, param); duration_ms = default_duration_milliseconds; } else { duration_ms = static_cast(parsed_duration); } } - - param = separator == nullptr ? nullptr : separator + 1; } if (output_file_name) { diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 85a0d63f4a7..0015e41d005 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -172,36 +172,31 @@ AndroidSystem::monodroid_get_system_property (const char *name, dynamic_local_st return Helpers::add_with_overflow_check (plen, 0); } -int +std::string_view AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept { - if (value == nullptr || value_size == 0) { - return -1; + if (value == nullptr || value_size < PROPERTY_VALUE_BUFFER_LEN) { + return {}; } value [0] = '\0'; - if (value_size < PROPERTY_VALUE_BUFFER_LEN) { - return -1; - } int len = _monodroid__system_property_get (name.data (), value, value_size); if (len > 0) { - return len; + return { value, static_cast(len) }; } size_t property_length; const char *property_value = lookup_system_property (name.data (), property_length); if (property_value == nullptr) { - return len; - } - - if (property_length >= value_size) { - return -1; + return {}; } - memcpy (value, property_value, property_length); - value [property_length] = '\0'; - return Helpers::add_with_overflow_check (property_length, 0); + // Bundled properties are NUL-terminated strings in static application data which live for as + // long as the process does, so we can return them directly instead of copying them into + // `value`. This also means that, unlike Android system properties, their length is not limited + // by `PROPERTY_VALUE_BUFFER_LEN`. + return { property_value, property_length }; } int diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index a38c0ac472e..d34632d4243 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -95,7 +95,13 @@ namespace xamarin::android::internal { static void create_update_dir (char *override_dir) noexcept; static int monodroid_get_system_property (const char *name, char **value) noexcept; static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; - static int monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; + // Returns a view of the property's value, or an empty view if the property is not set. + // + // `value` is a scratch buffer of at least `PROPERTY_VALUE_BUFFER_LEN` bytes, used to receive + // Android system properties. Build-time (bundled) properties are not copied into it - the + // returned view points at static application data instead, so their length is not limited by + // `value_size`. The returned value is always NUL-terminated. + static std::string_view monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; static int monodroid_get_system_property (std::string_view const& name, char **value) noexcept { From 69883f6323a5ae628c94ab5a3eaa42b6ef8c092e Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 09:51:31 +0200 Subject: [PATCH 15/26] [native] Return a NUL-terminated string from monodroid_get_system_property 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 --- src/native/clr/host/assembly-store.cc | 4 ++-- .../include/runtime-base/android-system.hh | 12 +++++++----- .../clr/runtime-base/android-system-shared.cc | 19 ++++++++++--------- src/native/clr/runtime-base/logger.cc | 6 ++++-- .../common/runtime-base/timing-internal.cc | 6 +++--- .../mono/runtime-base/android-system.cc | 11 ++++++----- .../mono/runtime-base/android-system.hh | 10 ++++++---- 7 files changed, 38 insertions(+), 30 deletions(-) diff --git a/src/native/clr/host/assembly-store.cc b/src/native/clr/host/assembly-store.cc index e7c1a4e5d2d..ceae90a11e3 100644 --- a/src/native/clr/host/assembly-store.cc +++ b/src/native/clr/host/assembly-store.cc @@ -297,8 +297,8 @@ namespace { } { char prop_value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - std::string_view cache_prop = AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)); - if (!cache_prop.empty ()) { + const char *cache_prop = AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)); + if (cache_prop != nullptr) { if (cache_prop [0] == '0') { cache_requested = false; } else if (cache_prop [0] == '1') { diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 6ac17b722ff..9dc7842c4bd 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -113,7 +113,7 @@ namespace xamarin::android { * pre-loaded apps!), we need the .__override__ directory... */ char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)).empty ()) [[likely]] { + if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) == nullptr) [[likely]] { return; } } @@ -128,13 +128,15 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - // Returns a view of the property's value, or an empty view if the property is not set. + // Returns the property's NUL-terminated value, or `nullptr` if the property is not set. // // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, // used to receive Android system properties. Build-time (bundled) properties are not - // copied into it - the returned view points at static application data instead, so their - // length is not limited by `value_size`. The returned value is always NUL-terminated. - static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> std::string_view; + // copied into it - the returned pointer refers to static application data instead, so + // their length is not limited by `value_size`. + // + // The result is valid for at least as long as `value` is. + static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> const char*; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; static void setup_app_library_directories (jstring_array_wrapper& runtimeApks, jstring_array_wrapper& appDirs, bool have_split_apks) noexcept; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index ecd120eeb35..a8af70afcc5 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -7,30 +7,31 @@ using namespace xamarin::android; using std::operator""sv; -auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> std::string_view +auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> const char* { if (value == nullptr || value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { - return {}; + return nullptr; } value [0] = '\0'; int len = monodroid__system_property_get (name, value, value_size); if (len > 0) { - return { value, static_cast(len) }; + // `__system_property_get` NUL-terminates the value it writes. + return value; } size_t property_length; const char *property_value = lookup_system_property (name, property_length); if (property_value == nullptr) { - return {}; + return nullptr; } // Bundled properties are NUL-terminated strings in static application data which live for as // long as the process does, so we can return them directly instead of copying them into // `value`. This also means that, unlike Android system properties, their length is not limited // by `Constants::PROPERTY_VALUE_BUFFER_LEN`. - return { property_value, property_length }; + return property_value; } auto @@ -69,10 +70,10 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long } char override[Constants::PROPERTY_VALUE_BUFFER_LEN]; - std::string_view grefc = monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)); - if (!grefc.empty ()) { + const char *grefc = monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)); + if (grefc != nullptr) { char *e; - max = strtol (grefc.data (), &e, 10); + max = strtol (grefc, &e, 10); switch (*e) { case 'k': e++; @@ -94,7 +95,7 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long "Unsupported '%.*s' value '%s'.", static_cast(Constants::DEBUG_MONO_MAX_GREFC.length ()), Constants::DEBUG_MONO_MAX_GREFC.data (), - grefc.data () + grefc ); } diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 3633ac7a93a..f10834c4f24 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -141,11 +141,13 @@ Logger::init_logging_categories () noexcept _log_timing_categories = LogTimingCategories::Default; char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - std::string_view remaining = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); - if (remaining.empty ()) { + const char *categories = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); + if (categories == nullptr) { return; } + std::string_view remaining { categories }; + while (!remaining.empty ()) { size_t separator = remaining.find (','); std::string_view param = remaining.substr (0, separator); diff --git a/src/native/common/runtime-base/timing-internal.cc b/src/native/common/runtime-base/timing-internal.cc index 44f36f3cf98..d7721fd7816 100644 --- a/src/native/common/runtime-base/timing-internal.cc +++ b/src/native/common/runtime-base/timing-internal.cc @@ -30,9 +30,9 @@ void FastTiming::really_initialize (bool log_immediately) noexcept } char value [Constants::PROPERTY_VALUE_BUFFER_LEN]; - std::string_view options = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)); - if (!options.empty ()) { - internal_timing.parse_options (options); + const char *options = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)); + if (options != nullptr) { + internal_timing.parse_options (std::string_view { options }); } log_write ( diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 0015e41d005..50bce03d282 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -172,31 +172,32 @@ AndroidSystem::monodroid_get_system_property (const char *name, dynamic_local_st return Helpers::add_with_overflow_check (plen, 0); } -std::string_view +const char* AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept { if (value == nullptr || value_size < PROPERTY_VALUE_BUFFER_LEN) { - return {}; + return nullptr; } value [0] = '\0'; int len = _monodroid__system_property_get (name.data (), value, value_size); if (len > 0) { - return { value, static_cast(len) }; + // `__system_property_get` NUL-terminates the value it writes. + return value; } size_t property_length; const char *property_value = lookup_system_property (name.data (), property_length); if (property_value == nullptr) { - return {}; + return nullptr; } // Bundled properties are NUL-terminated strings in static application data which live for as // long as the process does, so we can return them directly instead of copying them into // `value`. This also means that, unlike Android system properties, their length is not limited // by `PROPERTY_VALUE_BUFFER_LEN`. - return { property_value, property_length }; + return property_value; } int diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index d34632d4243..d9ec9f7fbd2 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -95,13 +95,15 @@ namespace xamarin::android::internal { static void create_update_dir (char *override_dir) noexcept; static int monodroid_get_system_property (const char *name, char **value) noexcept; static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; - // Returns a view of the property's value, or an empty view if the property is not set. + // Returns the property's NUL-terminated value, or `nullptr` if the property is not set. // // `value` is a scratch buffer of at least `PROPERTY_VALUE_BUFFER_LEN` bytes, used to receive // Android system properties. Build-time (bundled) properties are not copied into it - the - // returned view points at static application data instead, so their length is not limited by - // `value_size`. The returned value is always NUL-terminated. - static std::string_view monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; + // returned pointer refers to static application data instead, so their length is not limited + // by `value_size`. + // + // The result is valid for at least as long as `value` is. + static const char* monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; static int monodroid_get_system_property (std::string_view const& name, char **value) noexcept { From 26e12356bad1d2f5683fe72e2d17eaa6da4ea7b5 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 10:40:12 +0200 Subject: [PATCH 16/26] [native] Address the review feedback on the property and typemap changes - `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 `` 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 --- src/native/clr/host/assembly-store.cc | 2 +- src/native/clr/host/typemap.cc | 25 +++++------ .../clr/include/host/host-environment.hh | 4 +- .../include/runtime-base/android-system.hh | 18 ++++---- .../clr/runtime-base/android-system-shared.cc | 35 +++++---------- src/native/clr/runtime-base/android-system.cc | 4 +- src/native/clr/runtime-base/logger.cc | 3 +- .../include/runtime-base/timing-internal.hh | 2 +- .../common/runtime-base/timing-internal.cc | 44 +++++++++---------- .../mono/runtime-base/android-system.cc | 21 +++------ .../mono/runtime-base/android-system.hh | 12 +++-- .../nativeaot/runtime-base/android-system.cc | 2 +- 12 files changed, 72 insertions(+), 100 deletions(-) diff --git a/src/native/clr/host/assembly-store.cc b/src/native/clr/host/assembly-store.cc index ceae90a11e3..1639da5ef96 100644 --- a/src/native/clr/host/assembly-store.cc +++ b/src/native/clr/host/assembly-store.cc @@ -297,7 +297,7 @@ namespace { } { char prop_value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - const char *cache_prop = AndroidSystem::monodroid_get_system_property ("debug.net.asmcache"sv, prop_value, sizeof (prop_value)); + const char *cache_prop = AndroidSystem::monodroid_get_system_property ("debug.net.asmcache", prop_value, sizeof (prop_value)); if (cache_prop != nullptr) { if (cache_prop [0] == '0') { cache_requested = false; diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 552091453d4..1743767e7f8 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -21,24 +21,23 @@ namespace { return value_length == key_length && strncmp (value, key, key_length) == 0; } +#if defined (DEBUG) + // Returns the length of the formatted name, or the negative capacity the caller must provide + // when `buffer` is too small. auto format_managed_type_name (const char *type_name, const char *assembly_name, char *buffer, size_t buffer_size) noexcept -> ssize_t { - size_t type_name_length = strlen (type_name); - size_t assembly_name_length = strlen (assembly_name); - size_t full_name_length = Helpers::add_with_overflow_check (type_name_length, assembly_name_length); - full_name_length = Helpers::add_with_overflow_check (full_name_length, 2uz); - size_t required_capacity = Helpers::add_with_overflow_check (full_name_length, 1uz); - abort_unless (required_capacity <= static_cast(std::numeric_limits::max ()), "Managed type name is too long"); - if (buffer == nullptr || buffer_size < required_capacity) { - return -static_cast(required_capacity); + int full_name_length = snprintf (buffer, buffer_size, "%s, %s", type_name, assembly_name); + abort_unless (full_name_length >= 0, "Failed to format the managed type name"); + + size_t length = static_cast(full_name_length); + abort_unless (length < static_cast(std::numeric_limits::max ()), "Managed type name is too long"); + if (length >= buffer_size) { + return -static_cast(length + 1uz); } - memcpy (buffer, type_name, type_name_length); - memcpy (buffer + type_name_length, ", ", 2uz); - memcpy (buffer + type_name_length + 2uz, assembly_name, assembly_name_length); - buffer [full_name_length] = '\0'; - return static_cast(full_name_length); + return static_cast(length); } +#endif // def DEBUG class MonoGuidString { diff --git a/src/native/clr/include/host/host-environment.hh b/src/native/clr/include/host/host-environment.hh index 5d50fb46959..137beb468bd 100644 --- a/src/native/clr/include/host/host-environment.hh +++ b/src/native/clr/include/host/host-environment.hh @@ -52,7 +52,7 @@ namespace xamarin::android { } [[gnu::flatten, gnu::always_inline]] - static auto lookup_system_property (std::string_view const& name, size_t &value_len, + static auto lookup_system_property (const char *name, size_t &value_len, uint32_t const count, AppEnvironmentVariable const (&entries)[], const char (&contents)[]) noexcept -> const char* { @@ -63,7 +63,7 @@ namespace xamarin::android { for (size_t i = 0; i < count; i++) { AppEnvironmentVariable const& sys_prop = entries[i]; const char *prop_name = &contents[sys_prop.name_index]; - if (name.compare (prop_name) != 0) { + if (strcmp (name, prop_name) != 0) { continue; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 9dc7842c4bd..054893d2983 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -113,7 +113,7 @@ namespace xamarin::android { * pre-loaded apps!), we need the .__override__ directory... */ char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY, value, sizeof (value)) == nullptr) [[likely]] { + if (log_categories == 0 && monodroid_get_system_property (Constants::DEBUG_MONO_PROFILE_PROPERTY.data (), value, sizeof (value)) == nullptr) [[likely]] { return; } } @@ -128,15 +128,13 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - // Returns the property's NUL-terminated value, or `nullptr` if the property is not set. + // Returns the property's NUL-terminated value, or `nullptr` if it is not set. The result is + // valid for at least as long as `value` is. // - // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, - // used to receive Android system properties. Build-time (bundled) properties are not - // copied into it - the returned pointer refers to static application data instead, so + // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, used + // to receive Android system properties. Bundled properties are returned without copying, so // their length is not limited by `value_size`. - // - // The result is valid for at least as long as `value` is. - static auto monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> const char*; + static auto monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept -> const char*; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; static void setup_app_library_directories (jstring_array_wrapper& runtimeApks, jstring_array_wrapper& appDirs, bool have_split_apks) noexcept; @@ -149,8 +147,8 @@ namespace xamarin::android { static auto load_dso_from_specified_dirs (TContainer directories, std::string_view const& dso_name, int dl_flags, bool is_jni) noexcept -> void*; static auto load_dso_from_app_lib_dirs (std::string_view const& name, int dl_flags, bool is_jni) noexcept -> void*; static auto load_dso_from_override_dirs (std::string_view const& name, int dl_flags, bool is_jni) noexcept -> void*; - static auto lookup_system_property (std::string_view const &name, size_t &value_len) noexcept -> const char*; - static auto monodroid__system_property_get (std::string_view const&, char *sp_value, size_t sp_value_len) noexcept -> int; + static auto lookup_system_property (const char *name, size_t &value_len) noexcept -> const char*; + static auto monodroid__system_property_get (const char *name, char *sp_value, size_t sp_value_len) noexcept -> int; static auto get_max_gref_count_from_system () noexcept -> long; static void add_apk_libdir (std::string_view const& apk, size_t &index, std::string_view const& abi) noexcept; static void setup_apk_directories (unsigned short running_on_cpu, jstring_array_wrapper &runtimeApks, bool have_split_apks) noexcept; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index a8af70afcc5..5b876840d9d 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -1,13 +1,10 @@ #include -#include #include using namespace xamarin::android; -using std::operator""sv; - -auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept -> const char* +auto AndroidSystem::monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept -> const char* { if (value == nullptr || value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { return nullptr; @@ -15,29 +12,22 @@ auto AndroidSystem::monodroid_get_system_property (std::string_view const& name, value [0] = '\0'; - int len = monodroid__system_property_get (name, value, value_size); - if (len > 0) { - // `__system_property_get` NUL-terminates the value it writes. + // `__system_property_get` NUL-terminates what it writes. + if (monodroid__system_property_get (name, value, value_size) > 0) { return value; } + // Bundled properties are NUL-terminated strings in static application data which live as long + // as the process, so return them directly rather than copying them into `value`. Their length + // is therefore not limited by `Constants::PROPERTY_VALUE_BUFFER_LEN`. size_t property_length; - const char *property_value = lookup_system_property (name, property_length); - if (property_value == nullptr) { - return nullptr; - } - - // Bundled properties are NUL-terminated strings in static application data which live for as - // long as the process does, so we can return them directly instead of copying them into - // `value`. This also means that, unlike Android system properties, their length is not limited - // by `Constants::PROPERTY_VALUE_BUFFER_LEN`. - return property_value; + return lookup_system_property (name, property_length); } auto -AndroidSystem::monodroid__system_property_get (std::string_view const& name, char *sp_value, size_t sp_value_len) noexcept -> int +AndroidSystem::monodroid__system_property_get (const char *name, char *sp_value, size_t sp_value_len) noexcept -> int { - if (name.empty () || sp_value == nullptr) { + if (name == nullptr || *name == '\0' || sp_value == nullptr) { return -1; } @@ -48,7 +38,7 @@ AndroidSystem::monodroid__system_property_get (std::string_view const& name, cha buf = new char [alloc_size]; } - int len = __system_property_get (name.data (), buf ? buf : sp_value); + int len = __system_property_get (name, buf ? buf : sp_value); if (buf != nullptr) { strncpy (sp_value, buf, sp_value_len); sp_value [sp_value_len] = '\0'; @@ -70,7 +60,7 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long } char override[Constants::PROPERTY_VALUE_BUFFER_LEN]; - const char *grefc = monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC, override, sizeof (override)); + const char *grefc = monodroid_get_system_property (Constants::DEBUG_MONO_MAX_GREFC.data (), override, sizeof (override)); if (grefc != nullptr) { char *e; max = strtol (grefc, &e, 10); @@ -92,8 +82,7 @@ AndroidSystem::get_max_gref_count_from_system () noexcept -> long if (*e) { log_warnf ( LOG_GC, - "Unsupported '%.*s' value '%s'.", - static_cast(Constants::DEBUG_MONO_MAX_GREFC.length ()), + "Unsupported '%s' value '%s'.", Constants::DEBUG_MONO_MAX_GREFC.data (), grefc ); diff --git a/src/native/clr/runtime-base/android-system.cc b/src/native/clr/runtime-base/android-system.cc index bafb0276289..75bf35a1950 100644 --- a/src/native/clr/runtime-base/android-system.cc +++ b/src/native/clr/runtime-base/android-system.cc @@ -294,12 +294,12 @@ AndroidSystem::detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcep } auto -AndroidSystem::lookup_system_property (std::string_view const& name, size_t &value_len) noexcept -> const char* +AndroidSystem::lookup_system_property (const char *name, size_t &value_len) noexcept -> const char* { value_len = 0; #if defined (DEBUG) if (!bundled_properties.empty ()) { - auto prop_iter = bundled_properties.find (name.data ()); + auto prop_iter = bundled_properties.find (name); if (prop_iter != bundled_properties.end ()) { value_len = prop_iter->second.length (); return prop_iter->first.c_str (); diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index f10834c4f24..2bbc3e4685b 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include @@ -141,7 +140,7 @@ Logger::init_logging_categories () noexcept _log_timing_categories = LogTimingCategories::Default; char value[Constants::PROPERTY_VALUE_BUFFER_LEN]; - const char *categories = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY, value, sizeof (value)); + const char *categories = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_LOG_PROPERTY.data (), value, sizeof (value)); if (categories == nullptr) { return; } diff --git a/src/native/common/include/runtime-base/timing-internal.hh b/src/native/common/include/runtime-base/timing-internal.hh index 0f0b64ca800..fe0a3ee3dfe 100644 --- a/src/native/common/include/runtime-base/timing-internal.hh +++ b/src/native/common/include/runtime-base/timing-internal.hh @@ -487,7 +487,7 @@ namespace xamarin::android { } private: - void parse_options (std::string_view options) noexcept; + void parse_options (const char *options) noexcept; static void really_initialize (bool log_immediately) noexcept; [[gnu::always_inline]] diff --git a/src/native/common/runtime-base/timing-internal.cc b/src/native/common/runtime-base/timing-internal.cc index d7721fd7816..2d6799b9936 100644 --- a/src/native/common/runtime-base/timing-internal.cc +++ b/src/native/common/runtime-base/timing-internal.cc @@ -30,9 +30,9 @@ void FastTiming::really_initialize (bool log_immediately) noexcept } char value [Constants::PROPERTY_VALUE_BUFFER_LEN]; - const char *options = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING, value, sizeof (value)); + const char *options = AndroidSystem::monodroid_get_system_property (Constants::DEBUG_MONO_TIMING.data (), value, sizeof (value)); if (options != nullptr) { - internal_timing.parse_options (std::string_view { options }); + internal_timing.parse_options (options); } log_write ( @@ -42,37 +42,33 @@ void FastTiming::really_initialize (bool log_immediately) noexcept ); } -void FastTiming::parse_options (std::string_view options) noexcept +void FastTiming::parse_options (const char *options) noexcept { - while (!options.empty ()) { - size_t separator = options.find (','); - std::string_view param = options.substr (0, separator); - if (separator == std::string_view::npos) { - options = {}; - } else { - options.remove_prefix (separator + 1); - } - - if (param == OPT_TO_FILE) { + const char *param = options; + while (param != nullptr && *param != '\0') { + // The value may point at immortal bundled property data, so the parameters cannot be + // NUL-terminated in place. Bound every comparison by `param_length` instead. + const char *separator = strchr (param, ','); + size_t param_length = separator != nullptr ? static_cast(separator - param) : strlen (param); + + if (param_length == OPT_TO_FILE.length () && strncmp (param, OPT_TO_FILE.data (), param_length) == 0) { log_to_file = true; - } else if (param.starts_with (OPT_FILE_NAME)) { - output_file_name = std::make_unique (param.substr (OPT_FILE_NAME.length ())); - } else if (param.starts_with (OPT_DURATION)) { - std::string_view duration = param.substr (OPT_DURATION.length ()); - // `duration` is not NUL-terminated, but it is always followed by either the separator - // or the terminator of the whole options string, so `strtoull` cannot run past it. - const char *duration_start = duration.data (); - const char *duration_end = duration_start + duration.length (); + } else if (param_length >= OPT_FILE_NAME.length () && strncmp (param, OPT_FILE_NAME.data (), OPT_FILE_NAME.length ()) == 0) { + output_file_name = std::make_unique (param + OPT_FILE_NAME.length (), param_length - OPT_FILE_NAME.length ()); + } else if (param_length >= OPT_DURATION.length () && strncmp (param, OPT_DURATION.data (), OPT_DURATION.length ()) == 0) { + const char *duration = param + OPT_DURATION.length (); char *end; errno = 0; - unsigned long long parsed_duration = strtoull (duration_start, &end, 10); - if (end != duration_end || duration.empty () || errno == ERANGE || parsed_duration > std::numeric_limits::max ()) { - log_warn (LOG_TIMING, "Failed to parse duration in milliseconds from '{}'"sv, param); + unsigned long long parsed_duration = strtoull (duration, &end, 10); + if (end == duration || end != param + param_length || errno == ERANGE || parsed_duration > std::numeric_limits::max ()) { + log_warnf (LOG_TIMING, "Failed to parse duration in milliseconds from '%.*s'", static_cast(param_length), param); duration_ms = default_duration_milliseconds; } else { duration_ms = static_cast(parsed_duration); } } + + param = separator == nullptr ? nullptr : separator + 1; } if (output_file_name) { diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 50bce03d282..41ee9e16baa 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -173,7 +173,7 @@ AndroidSystem::monodroid_get_system_property (const char *name, dynamic_local_st } const char* -AndroidSystem::monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept +AndroidSystem::monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept { if (value == nullptr || value_size < PROPERTY_VALUE_BUFFER_LEN) { return nullptr; @@ -181,23 +181,16 @@ AndroidSystem::monodroid_get_system_property (std::string_view const& name, char value [0] = '\0'; - int len = _monodroid__system_property_get (name.data (), value, value_size); - if (len > 0) { - // `__system_property_get` NUL-terminates the value it writes. + // `__system_property_get` NUL-terminates what it writes. + if (_monodroid__system_property_get (name, value, value_size) > 0) { return value; } + // Bundled properties are NUL-terminated strings in static application data which live as long + // as the process, so return them directly rather than copying them into `value`. Their length + // is therefore not limited by `PROPERTY_VALUE_BUFFER_LEN`. size_t property_length; - const char *property_value = lookup_system_property (name.data (), property_length); - if (property_value == nullptr) { - return nullptr; - } - - // Bundled properties are NUL-terminated strings in static application data which live for as - // long as the process does, so we can return them directly instead of copying them into - // `value`. This also means that, unlike Android system properties, their length is not limited - // by `PROPERTY_VALUE_BUFFER_LEN`. - return property_value; + return lookup_system_property (name, property_length); } int diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index d9ec9f7fbd2..4e7585d9ca3 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -95,15 +95,13 @@ namespace xamarin::android::internal { static void create_update_dir (char *override_dir) noexcept; static int monodroid_get_system_property (const char *name, char **value) noexcept; static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; - // Returns the property's NUL-terminated value, or `nullptr` if the property is not set. + // Returns the property's NUL-terminated value, or `nullptr` if it is not set. The result is + // valid for at least as long as `value` is. // // `value` is a scratch buffer of at least `PROPERTY_VALUE_BUFFER_LEN` bytes, used to receive - // Android system properties. Build-time (bundled) properties are not copied into it - the - // returned pointer refers to static application data instead, so their length is not limited - // by `value_size`. - // - // The result is valid for at least as long as `value` is. - static const char* monodroid_get_system_property (std::string_view const& name, char *value, size_t value_size) noexcept; + // Android system properties. Bundled properties are returned without copying, so their + // length is not limited by `value_size`. + static const char* monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept; static int monodroid_get_system_property (std::string_view const& name, char **value) noexcept { diff --git a/src/native/nativeaot/runtime-base/android-system.cc b/src/native/nativeaot/runtime-base/android-system.cc index 52e19c153bc..f9ff09d3671 100644 --- a/src/native/nativeaot/runtime-base/android-system.cc +++ b/src/native/nativeaot/runtime-base/android-system.cc @@ -6,7 +6,7 @@ using namespace xamarin::android; -auto AndroidSystem::lookup_system_property (std::string_view const& name, size_t &value_len) noexcept -> const char* +auto AndroidSystem::lookup_system_property (const char *name, size_t &value_len) noexcept -> const char* { return HostEnvironment::lookup_system_property ( name, From 4f80d369a203f2ddea63c50f792696909fcd61b2 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 10:54:37 +0200 Subject: [PATCH 17/26] [native] Parse the logging categories without std::string_view `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 --- src/native/clr/include/runtime-base/logger.hh | 6 +- src/native/clr/runtime-base/logger.cc | 167 ++++++++---------- 2 files changed, 74 insertions(+), 99 deletions(-) diff --git a/src/native/clr/include/runtime-base/logger.hh b/src/native/clr/include/runtime-base/logger.hh index 9bf67148fc1..7573271b042 100644 --- a/src/native/clr/include/runtime-base/logger.hh +++ b/src/native/clr/include/runtime-base/logger.hh @@ -49,11 +49,11 @@ namespace xamarin::android { } private: - static auto open_file (std::string_view const& path) noexcept -> FILE*; - static auto open_file (LogCategories category, std::string_view const& custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE*; + static auto open_file (const char *path) noexcept -> FILE*; + static auto open_file (LogCategories category, const char *custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE*; private: - static bool set_category (std::string_view const& name, std::string_view const& arg, unsigned int entry, bool arg_starts_with_name = false) noexcept; + static bool set_category (const char *name, const char *arg, size_t arg_length, unsigned int entry, bool arg_starts_with_name = false) noexcept; private: static inline LogTimingCategories _log_timing_categories; diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 2bbc3e4685b..b7c0557f0aa 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -25,16 +25,27 @@ namespace { bool light_gref = false; bool light_lref = false; - void set_log_file (char *&log_file, std::string_view path) noexcept + // Compares a comma-separated parameter, which is not NUL-terminated, against `text`. + bool param_matches (const char *param, size_t param_length, const char *text, bool prefix_only = false) noexcept + { + size_t text_length = strlen (text); + if (prefix_only ? param_length < text_length : param_length != text_length) { + return false; + } + + return strncmp (param, text, text_length) == 0; + } + + void set_log_file (char *&log_file, const char *path, size_t path_length) noexcept { char *new_log_file = nullptr; - if (!path.empty ()) { - size_t allocation_size = Helpers::add_with_overflow_check (path.length (), 1uz); + if (path != nullptr && path_length > 0) { + size_t allocation_size = Helpers::add_with_overflow_check (path_length, 1uz); new_log_file = static_cast (std::malloc (allocation_size)); abort_unless (new_log_file != nullptr, "Failed to allocate reference log file path"); - memcpy (new_log_file, path.data (), path.length ()); - new_log_file [path.length ()] = '\0'; + memcpy (new_log_file, path, path_length); + new_log_file [path_length] = '\0'; } std::free (log_file); @@ -43,14 +54,14 @@ namespace { } [[gnu::always_inline]] -auto Logger::open_file (std::string_view const& path) noexcept -> FILE* +auto Logger::open_file (const char *path) noexcept -> FILE* { - if (path.empty ()) { + if (path == nullptr || *path == '\0') { return nullptr; } // Ignore errors, by design - unlink (path.data ()); + unlink (path); // `monodroid_fopen` will log any errors FILE *ret = Util::monodroid_fopen (path, "a"sv); @@ -62,11 +73,11 @@ auto Logger::open_file (std::string_view const& path) noexcept -> FILE* } [[gnu::flatten, gnu::always_inline]] -auto Logger::open_file (LogCategories category, std::string_view const& custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE* +auto Logger::open_file (LogCategories category, const char *custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE* { - auto log_and_return = [&category](FILE *f, std::string_view const& path) -> FILE* { + auto log_and_return = [&category](FILE *f, const char *path) -> FILE* { if (f != nullptr) { - log_debugf (category, "Opened file '%.*s' for logging.", static_cast(path.length ()), path.data ()); + log_debugf (category, "Opened file '%s' for logging.", path); } return f; }; @@ -79,8 +90,7 @@ auto Logger::open_file (LogCategories category, std::string_view const& custom_p char stack_buffer [Util::LocalPathBufferSize]; char *path_buffer = Util::join_paths (stack_buffer, sizeof (stack_buffer), override_dir, fallback_filename); - std::string_view path_view { path_buffer }; - ret = log_and_return (open_file (path_view), path_view); + ret = log_and_return (open_file (path_buffer), path_buffer); if (path_buffer != stack_buffer) { std::free (path_buffer); } @@ -91,12 +101,7 @@ void Logger::init_reference_logging (std::string_view const& override_dir) noexcept { if ((log_categories & LOG_GREF) != 0 && !light_gref) { - _gref_log = open_file ( - LOG_GREF, - gref_file == nullptr ? std::string_view {} : std::string_view { gref_file }, - override_dir, - "grefs.txt"sv - ); + _gref_log = open_file (LOG_GREF, gref_file, override_dir, "grefs.txt"sv); } if ((log_categories & LOG_LREF) != 0 && !light_lref) { @@ -104,12 +109,7 @@ Logger::init_reference_logging (std::string_view const& override_dir) noexcept if (lref_file != nullptr && strcmp (lref_file, gref_file != nullptr ? gref_file : "") == 0) { _lref_log = _gref_log; } else { - _lref_log = open_file ( - LOG_LREF, - lref_file == nullptr ? std::string_view {} : std::string_view { lref_file }, - override_dir, - "lrefs.txt"sv - ); + _lref_log = open_file (LOG_LREF, lref_file, override_dir, "lrefs.txt"sv); } } @@ -120,13 +120,13 @@ Logger::init_reference_logging (std::string_view const& override_dir) noexcept } [[gnu::always_inline]] bool -Logger::set_category (std::string_view const& name, std::string_view const& arg, unsigned int entry, bool arg_starts_with_name) noexcept +Logger::set_category (const char *name, const char *arg, size_t arg_length, unsigned int entry, bool arg_starts_with_name) noexcept { if ((log_categories & entry) == entry) { return false; } - if (arg_starts_with_name ? arg.starts_with (name) : arg == name) { + if (param_matches (arg, arg_length, name, arg_starts_with_name)) { log_categories |= entry; return true; } @@ -145,117 +145,92 @@ Logger::init_logging_categories () noexcept return; } - std::string_view remaining { categories }; - - while (!remaining.empty ()) { - size_t separator = remaining.find (','); - std::string_view param = remaining.substr (0, separator); - if (separator == std::string_view::npos) { - remaining = {}; - } else { - remaining.remove_prefix (separator + 1); - } - - constexpr std::string_view CAT_ALL { "all" }; + // The value may point at immortal bundled property data, so the parameters cannot be + // NUL-terminated in place. Bound every comparison by `param_length` instead. + const char *param = categories; + while (param != nullptr && *param != '\0') { + const char *separator = strchr (param, ','); + size_t param_length = separator != nullptr ? static_cast(separator - param) : strlen (param); + const char *next = separator == nullptr ? nullptr : separator + 1; - if (param == CAT_ALL) { + if (param_matches (param, param_length, "all")) { log_categories = 0xFFFFFFFF; break; } - if (set_category ("assembly", param, LOG_ASSEMBLY)) { - continue; - } - - if (set_category ("default", param, LOG_DEFAULT)) { - continue; - } - - if (set_category ("debugger", param, LOG_DEBUGGER)) { - continue; - } - - if (set_category ("gc", param, LOG_GC)) { + if (set_category ("assembly", param, param_length, LOG_ASSEMBLY) || + set_category ("default", param, param_length, LOG_DEFAULT) || + set_category ("debugger", param, param_length, LOG_DEBUGGER) || + set_category ("gc", param, param_length, LOG_GC) || + set_category ("gref", param, param_length, LOG_GREF) || + set_category ("lref", param, param_length, LOG_LREF) || + set_category ("timing", param, param_length, LOG_TIMING) || + set_category ("network", param, param_length, LOG_NET) || + set_category ("netlink", param, param_length, LOG_NETLINK)) { + param = next; continue; } - if (set_category ("gref", param, LOG_GREF)) { - continue; - } - - if (set_category ("lref", param, LOG_LREF)) { - continue; - } - - if (set_category ("timing", param, LOG_TIMING)) { - continue; - } - - if (set_category ("network", param, LOG_NET)) { - continue; - } - - if (set_category ("netlink", param, LOG_NETLINK)) { - continue; - } - - auto get_log_file_name = [](std::string_view const& file_kind, std::string_view const& segment, size_t offset) -> std::string_view { - if (offset >= segment.length ()) { - log_warnf ( - LOG_DEFAULT, - "Unable to set path to %.*s log file: %s", - static_cast(file_kind.length ()), - file_kind.data (), - "no file name specified" - ); - return {}; + auto set_log_file_from_param = [param, param_length](char *&log_file, const char *file_kind) { + constexpr size_t OFFSET = 5uz; // The length of the "gref=" and "lref=" prefixes. + if (OFFSET >= param_length) { + log_warnf (LOG_DEFAULT, "Unable to set path to %s log file: no file name specified", file_kind); + set_log_file (log_file, nullptr, 0uz); + return; } - return segment.substr (offset); + set_log_file (log_file, param + OFFSET, param_length - OFFSET); }; - constexpr std::string_view CAT_GREF_EQUALS { "gref=" }; - if (set_category (CAT_GREF_EQUALS, param, LOG_GREF, true /* arg_starts_with_name */)) { - set_log_file (gref_file, get_log_file_name ("gref"sv, param, CAT_GREF_EQUALS.length ())); + if (set_category ("gref=", param, param_length, LOG_GREF, true /* arg_starts_with_name */)) { + set_log_file_from_param (gref_file, "gref"); + param = next; continue; } - if (set_category ("gref-", param, LOG_GREF)) { + if (set_category ("gref-", param, param_length, LOG_GREF)) { light_gref = true; + param = next; continue; } - if (set_category ("gref+", param, LOG_GREF)) { + if (set_category ("gref+", param, param_length, LOG_GREF)) { _gref_to_logcat = true; + param = next; continue; } - constexpr std::string_view CAT_LREF_EQUALS { "lref=" }; - if (set_category (CAT_LREF_EQUALS, param, LOG_LREF, true /* arg_starts_with_name */)) { - set_log_file (lref_file, get_log_file_name ("lref"sv, param, CAT_LREF_EQUALS.length ())); + if (set_category ("lref=", param, param_length, LOG_LREF, true /* arg_starts_with_name */)) { + set_log_file_from_param (lref_file, "lref"); + param = next; continue; } - if (set_category ("lref-", param, LOG_LREF)) { + if (set_category ("lref-", param, param_length, LOG_LREF)) { light_lref = true; + param = next; continue; } - if (set_category ("lref+", param, LOG_LREF)) { + if (set_category ("lref+", param, param_length, LOG_LREF)) { _lref_to_logcat = true; + param = next; continue; } - if (param.starts_with ("timing=fast-bare")) { + if (param_matches (param, param_length, "timing=fast-bare", true /* prefix_only */)) { log_categories |= LOG_TIMING; _log_timing_categories |= LogTimingCategories::FastBare; + param = next; continue; } - if (param.starts_with ("timing=bare")) { + if (param_matches (param, param_length, "timing=bare", true /* prefix_only */)) { log_categories |= LOG_TIMING; _log_timing_categories |= LogTimingCategories::Bare; } + + param = next; } if ((log_categories & LOG_GC) != 0) { From 2d30854e7221660b3ac7660f81ca81f9a17c39da Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 11:50:36 +0200 Subject: [PATCH 18/26] [native] Document that monodroid_get_system_property never allocates 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 --- src/native/clr/include/runtime-base/android-system.hh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 054893d2983..70acf607500 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -128,12 +128,14 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - // Returns the property's NUL-terminated value, or `nullptr` if it is not set. The result is - // valid for at least as long as `value` is. + // Returns the property's NUL-terminated value, or `nullptr` if it is not set. // // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, used // to receive Android system properties. Bundled properties are returned without copying, so // their length is not limited by `value_size`. + // + // Nothing is ever allocated: the result is either `value` or a pointer to application data + // which lives as long as the process. The caller must not free it. static auto monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept -> const char*; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; From e265b33330f58a2d50ad34354670f86208b87315 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 12:26:38 +0200 Subject: [PATCH 19/26] [native] Take file system paths as C strings in Util and Logger `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 --- src/native/clr/host/host.cc | 2 +- .../include/runtime-base/android-system.hh | 2 +- src/native/clr/include/runtime-base/logger.hh | 6 +-- src/native/clr/include/runtime-base/util.hh | 6 +-- src/native/clr/runtime-base/logger.cc | 11 +++--- src/native/clr/runtime-base/util.cc | 38 +++++-------------- 6 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/native/clr/host/host.cc b/src/native/clr/host/host.cc index 1b06742d790..8f6b339bbcd 100644 --- a/src/native/clr/host/host.cc +++ b/src/native/clr/host/host.cc @@ -333,7 +333,7 @@ void Host::Java_mono_android_Runtime_initInternal ( AndroidSystem::set_app_code_cache_dir (applicationDirs[Constants::APP_DIRS_CODE_CACHE_DIR_INDEX]); AndroidSystem::create_update_dir (AndroidSystem::get_primary_override_dir ()); AndroidSystem::setup_environment (); - Logger::init_reference_logging (AndroidSystem::get_primary_override_dir ()); + Logger::init_reference_logging (AndroidSystem::get_primary_override_dir ().c_str ()); jstring_array_wrapper runtimeApks (env, runtimeApksJava); AndroidSystem::setup_app_library_directories (runtimeApks, applicationDirs, haveSplitApks); diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 70acf607500..346d2f06d96 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -119,7 +119,7 @@ namespace xamarin::android { } log_debug (LOG_DEFAULT, "Creating public update directory: `{}`", override_dir); - Util::create_public_directory (override_dir); + Util::create_public_directory (override_dir.c_str ()); } #endif diff --git a/src/native/clr/include/runtime-base/logger.hh b/src/native/clr/include/runtime-base/logger.hh index 7573271b042..f406570dfa8 100644 --- a/src/native/clr/include/runtime-base/logger.hh +++ b/src/native/clr/include/runtime-base/logger.hh @@ -2,8 +2,6 @@ #include -#include - #include namespace xamarin::android { @@ -11,7 +9,7 @@ namespace xamarin::android { { public: static void init_logging_categories () noexcept; - static void init_reference_logging (std::string_view const& override_dir) noexcept; + static void init_reference_logging (const char *override_dir) noexcept; static auto log_timing_categories () noexcept -> LogTimingCategories { @@ -50,7 +48,7 @@ namespace xamarin::android { private: static auto open_file (const char *path) noexcept -> FILE*; - static auto open_file (LogCategories category, const char *custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE*; + static auto open_file (LogCategories category, const char *custom_path, const char *override_dir, const char *fallback_filename) noexcept -> FILE*; private: static bool set_category (const char *name, const char *arg, size_t arg_length, unsigned int entry, bool arg_starts_with_name = false) noexcept; diff --git a/src/native/clr/include/runtime-base/util.hh b/src/native/clr/include/runtime-base/util.hh index ea6280418db..22cef78e193 100644 --- a/src/native/clr/include/runtime-base/util.hh +++ b/src/native/clr/include/runtime-base/util.hh @@ -48,9 +48,9 @@ namespace xamarin::android { return create_directory (dir.data (), mode); } - static void create_public_directory (std::string_view const& dir); - static auto monodroid_fopen (std::string_view const& filename, std::string_view const& mode) noexcept -> FILE*; - static void set_world_accessable (std::string_view const& path); + static void create_public_directory (const char *dir); + static auto monodroid_fopen (const char *filename, const char *mode) noexcept -> FILE*; + static void set_world_accessable (const char *path); static auto set_world_accessible (int fd) noexcept -> bool; // Puts higher half of the `value` byte as a hexadecimal character in `high_half` and diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index b7c0557f0aa..1206c42f4ea 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -17,7 +17,6 @@ #include using namespace xamarin::android; -using std::operator""sv; namespace { char *gref_file = nullptr; @@ -64,7 +63,7 @@ auto Logger::open_file (const char *path) noexcept -> FILE* unlink (path); // `monodroid_fopen` will log any errors - FILE *ret = Util::monodroid_fopen (path, "a"sv); + FILE *ret = Util::monodroid_fopen (path, "a"); if (ret != nullptr) { Util::set_world_accessable (path); } @@ -73,7 +72,7 @@ auto Logger::open_file (const char *path) noexcept -> FILE* } [[gnu::flatten, gnu::always_inline]] -auto Logger::open_file (LogCategories category, const char *custom_path, std::string_view const& override_dir, std::string_view const& fallback_filename) noexcept -> FILE* +auto Logger::open_file (LogCategories category, const char *custom_path, const char *override_dir, const char *fallback_filename) noexcept -> FILE* { auto log_and_return = [&category](FILE *f, const char *path) -> FILE* { if (f != nullptr) { @@ -98,10 +97,10 @@ auto Logger::open_file (LogCategories category, const char *custom_path, std::st } void -Logger::init_reference_logging (std::string_view const& override_dir) noexcept +Logger::init_reference_logging (const char *override_dir) noexcept { if ((log_categories & LOG_GREF) != 0 && !light_gref) { - _gref_log = open_file (LOG_GREF, gref_file, override_dir, "grefs.txt"sv); + _gref_log = open_file (LOG_GREF, gref_file, override_dir, "grefs.txt"); } if ((log_categories & LOG_LREF) != 0 && !light_lref) { @@ -109,7 +108,7 @@ Logger::init_reference_logging (std::string_view const& override_dir) noexcept if (lref_file != nullptr && strcmp (lref_file, gref_file != nullptr ? gref_file : "") == 0) { _lref_log = _gref_log; } else { - _lref_log = open_file (LOG_LREF, lref_file, override_dir, "lrefs.txt"sv); + _lref_log = open_file (LOG_LREF, lref_file, override_dir, "lrefs.txt"); } } diff --git a/src/native/clr/runtime-base/util.cc b/src/native/clr/runtime-base/util.cc index e7878539134..df5028fe807 100644 --- a/src/native/clr/runtime-base/util.cc +++ b/src/native/clr/runtime-base/util.cc @@ -50,63 +50,45 @@ Util::create_directory (const char *pathname, mode_t mode) } void -Util::create_public_directory (std::string_view const& dir) +Util::create_public_directory (const char *dir) { mode_t m = umask (0); - int ret = create_directory (dir.data (), 0777); + int ret = create_directory (dir, 0777); if (ret < 0) { if (errno == EEXIST) { // Try to change the mode, just in case - chmod (dir.data (), 0777); + chmod (dir, 0777); } else { - log_warnf ( - LOG_DEFAULT, - "Failed to create directory '%.*s'. %s", - static_cast(dir.length ()), - dir.data (), - std::strerror (errno) - ); + log_warnf (LOG_DEFAULT, "Failed to create directory '%s'. %s", dir, std::strerror (errno)); } } umask (m); } auto -Util::monodroid_fopen (std::string_view const& filename, std::string_view const& mode) noexcept -> FILE* +Util::monodroid_fopen (const char *filename, const char *mode) noexcept -> FILE* { /* On Unix, both path and system calls are all assumed * to be UTF-8 compliant. */ - FILE *ret = fopen (filename.data (), mode.data ()); + FILE *ret = fopen (filename, mode); if (ret == nullptr) { - log_errorf ( - LOG_DEFAULT, - "fopen failed for file %.*s: %s", - static_cast(filename.length ()), - filename.data (), - strerror (errno) - ); + log_errorf (LOG_DEFAULT, "fopen failed for file %s: %s", filename, strerror (errno)); return nullptr; } return ret; } -void Util::set_world_accessable (std::string_view const& path) +void Util::set_world_accessable (const char *path) { int r; do { - r = chmod (path.data (), 0664); + r = chmod (path, 0664); } while (r == -1 && errno == EINTR); if (r == -1) { - log_errorf ( - LOG_DEFAULT, - "chmod(\"%.*s\", 0664) failed: %s", - static_cast(path.length ()), - path.data (), - strerror (errno) - ); + log_errorf (LOG_DEFAULT, "chmod(\"%s\", 0664) failed: %s", path, strerror (errno)); } } From 1496159c9bf6fb14ab5dda8c9bd4996a9cee125c Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 14:00:51 +0200 Subject: [PATCH 20/26] [native] Fix bundled system property lookup and drop dead property code 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 --- .../include/runtime-base/android-system.hh | 2 +- .../clr/runtime-base/android-system-shared.cc | 30 +++++++------------ src/native/clr/runtime-base/android-system.cc | 2 +- src/native/clr/runtime-base/logger.cc | 2 +- 4 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 346d2f06d96..dce07a4d970 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -150,7 +150,7 @@ namespace xamarin::android { static auto load_dso_from_app_lib_dirs (std::string_view const& name, int dl_flags, bool is_jni) noexcept -> void*; static auto load_dso_from_override_dirs (std::string_view const& name, int dl_flags, bool is_jni) noexcept -> void*; static auto lookup_system_property (const char *name, size_t &value_len) noexcept -> const char*; - static auto monodroid__system_property_get (const char *name, char *sp_value, size_t sp_value_len) noexcept -> int; + static auto monodroid__system_property_get (const char *name, char *sp_value) noexcept -> int; static auto get_max_gref_count_from_system () noexcept -> long; static void add_apk_libdir (std::string_view const& apk, size_t &index, std::string_view const& abi) noexcept; static void setup_apk_directories (unsigned short running_on_cpu, jstring_array_wrapper &runtimeApks, bool have_split_apks) noexcept; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index 5b876840d9d..e7f05cf97ca 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -6,14 +6,17 @@ using namespace xamarin::android; auto AndroidSystem::monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept -> const char* { - if (value == nullptr || value_size < Constants::PROPERTY_VALUE_BUFFER_LEN) { - return nullptr; - } + // `__system_property_get` always writes up to `PROPERTY_VALUE_BUFFER_LEN` bytes, so a smaller + // buffer would overflow. This is a programming error, not a runtime condition. + abort_unless ( + value != nullptr && value_size >= Constants::PROPERTY_VALUE_BUFFER_LEN, + "System property value buffer is too small" + ); value [0] = '\0'; // `__system_property_get` NUL-terminates what it writes. - if (monodroid__system_property_get (name, value, value_size) > 0) { + if (monodroid__system_property_get (name, value) > 0) { return value; } @@ -25,27 +28,14 @@ auto AndroidSystem::monodroid_get_system_property (const char *name, char *value } auto -AndroidSystem::monodroid__system_property_get (const char *name, char *sp_value, size_t sp_value_len) noexcept -> int +AndroidSystem::monodroid__system_property_get (const char *name, char *sp_value) noexcept -> int { if (name == nullptr || *name == '\0' || sp_value == nullptr) { return -1; } - char *buf = nullptr; - if (sp_value_len < Constants::PROPERTY_VALUE_BUFFER_LEN) { - size_t alloc_size = Helpers::add_with_overflow_check (Constants::PROPERTY_VALUE_BUFFER_LEN, 1uz); - log_warnf (LOG_DEFAULT, "Buffer to store system property may be too small, will copy only %zu bytes", sp_value_len); - buf = new char [alloc_size]; - } - - int len = __system_property_get (name, buf ? buf : sp_value); - if (buf != nullptr) { - strncpy (sp_value, buf, sp_value_len); - sp_value [sp_value_len] = '\0'; - delete[] buf; - } - - return len; + // The caller guarantees that `sp_value` is at least `PROPERTY_VALUE_BUFFER_LEN` bytes long. + return __system_property_get (name, sp_value); } auto diff --git a/src/native/clr/runtime-base/android-system.cc b/src/native/clr/runtime-base/android-system.cc index 75bf35a1950..b8ce6fb56c4 100644 --- a/src/native/clr/runtime-base/android-system.cc +++ b/src/native/clr/runtime-base/android-system.cc @@ -302,7 +302,7 @@ AndroidSystem::lookup_system_property (const char *name, size_t &value_len) noex auto prop_iter = bundled_properties.find (name); if (prop_iter != bundled_properties.end ()) { value_len = prop_iter->second.length (); - return prop_iter->first.c_str (); + return prop_iter->second.c_str (); } } #endif // DEBUG diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 1206c42f4ea..9a1d04bee73 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -171,7 +171,7 @@ Logger::init_logging_categories () noexcept } auto set_log_file_from_param = [param, param_length](char *&log_file, const char *file_kind) { - constexpr size_t OFFSET = 5uz; // The length of the "gref=" and "lref=" prefixes. + constexpr size_t OFFSET = sizeof ("gref=") - 1; // Both the "gref=" and "lref=" prefixes are this long. if (OFFSET >= param_length) { log_warnf (LOG_DEFAULT, "Unable to set path to %s log file: no file name specified", file_kind); set_log_file (log_file, nullptr, 0uz); From 0f60bd69475a40f17c57837a462596df68c7e936 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 17:38:42 +0200 Subject: [PATCH 21/26] [native] Return the value, not the name, from Mono's bundled property 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 --- src/native/mono/runtime-base/android-system.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 41ee9e16baa..54dc7f4434c 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -58,7 +58,7 @@ AndroidSystem::lookup_system_property (const char *name, size_t &value_len) noex BundledProperty *p = lookup_system_property (name); if (p != nullptr) { value_len = p->value_len; - return p->name; + return p->value; } #endif // DEBUG || !ANDROID From db24706974f2853a6661ff8d2fae9500aba17761 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 18:12:32 +0200 Subject: [PATCH 22/26] [native] Report empty bundled properties as unset `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 --- src/native/clr/include/runtime-base/android-system.hh | 4 +++- src/native/clr/runtime-base/android-system-shared.cc | 11 ++++++++++- src/native/mono/runtime-base/android-system.cc | 11 ++++++++++- src/native/mono/runtime-base/android-system.hh | 5 +++-- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index dce07a4d970..c67d8ac3957 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -128,7 +128,9 @@ namespace xamarin::android { return embedded_dso_mode_enabled; } - // Returns the property's NUL-terminated value, or `nullptr` if it is not set. + // Returns the property's NUL-terminated value, or `nullptr` if it is not set. A property + // that is set to an empty value is reported as not set, matching `__system_property_get`, + // which cannot tell the two apart. // // `value` is a scratch buffer of at least `Constants::PROPERTY_VALUE_BUFFER_LEN` bytes, used // to receive Android system properties. Bundled properties are returned without copying, so diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index e7f05cf97ca..3ad74670d25 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -23,8 +23,17 @@ auto AndroidSystem::monodroid_get_system_property (const char *name, char *value // Bundled properties are NUL-terminated strings in static application data which live as long // as the process, so return them directly rather than copying them into `value`. Their length // is therefore not limited by `Constants::PROPERTY_VALUE_BUFFER_LEN`. + // + // A bundled property may be present but empty. `__system_property_get` cannot distinguish an + // empty value from a missing one either, so report both as unset and let callers keep their + // defaults. size_t property_length; - return lookup_system_property (name, property_length); + const char *bundled_value = lookup_system_property (name, property_length); + if (bundled_value == nullptr || property_length == 0) { + return nullptr; + } + + return bundled_value; } auto diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 54dc7f4434c..38ec215d561 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -189,8 +189,17 @@ AndroidSystem::monodroid_get_system_property (const char *name, char *value, siz // Bundled properties are NUL-terminated strings in static application data which live as long // as the process, so return them directly rather than copying them into `value`. Their length // is therefore not limited by `PROPERTY_VALUE_BUFFER_LEN`. + // + // A bundled property may be present but empty. `__system_property_get` cannot distinguish an + // empty value from a missing one either, so report both as unset and let callers keep their + // defaults. size_t property_length; - return lookup_system_property (name, property_length); + const char *bundled_value = lookup_system_property (name, property_length); + if (bundled_value == nullptr || property_length == 0) { + return nullptr; + } + + return bundled_value; } int diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index 4e7585d9ca3..9299b7b5155 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -95,8 +95,9 @@ namespace xamarin::android::internal { static void create_update_dir (char *override_dir) noexcept; static int monodroid_get_system_property (const char *name, char **value) noexcept; static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; - // Returns the property's NUL-terminated value, or `nullptr` if it is not set. The result is - // valid for at least as long as `value` is. + // Returns the property's NUL-terminated value, or `nullptr` if it is not set. A property + // that is set to an empty value is reported as not set, matching `__system_property_get`, + // which cannot tell the two apart. The result is valid for at least as long as `value` is. // // `value` is a scratch buffer of at least `PROPERTY_VALUE_BUFFER_LEN` bytes, used to receive // Android system properties. Bundled properties are returned without copying, so their From eb95617e9bcdfd4bd723b132072c5ab2be3c9f16 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 18:18:40 +0200 Subject: [PATCH 23/26] [native] Address self-review findings * `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 --- src/native/clr/include/runtime-base/android-system.hh | 3 ++- src/native/clr/include/runtime-base/logger.hh | 2 +- src/native/clr/runtime-base/logger.cc | 5 +++++ src/native/mono/runtime-base/android-system.cc | 9 ++++++--- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index c67d8ac3957..f2b2a0f76a5 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -169,6 +169,7 @@ namespace xamarin::android { static auto format_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept -> ssize_t { + abort_unless (buffer != nullptr, "Primary override directory buffer must not be null"); int length = snprintf ( buffer, buffer_size, @@ -182,7 +183,7 @@ namespace xamarin::android { abort_unless (length >= 0, "Failed to format primary override directory path"); size_t required_capacity = Helpers::add_with_overflow_check (static_cast(length), 1uz); abort_unless (required_capacity <= static_cast(std::numeric_limits::max ()), "Primary override directory path is too long"); - if (buffer == nullptr || buffer_size < required_capacity) { + if (buffer_size < required_capacity) { return -static_cast(required_capacity); } return static_cast(length); diff --git a/src/native/clr/include/runtime-base/logger.hh b/src/native/clr/include/runtime-base/logger.hh index f406570dfa8..4d7020aee0d 100644 --- a/src/native/clr/include/runtime-base/logger.hh +++ b/src/native/clr/include/runtime-base/logger.hh @@ -51,7 +51,7 @@ namespace xamarin::android { static auto open_file (LogCategories category, const char *custom_path, const char *override_dir, const char *fallback_filename) noexcept -> FILE*; private: - static bool set_category (const char *name, const char *arg, size_t arg_length, unsigned int entry, bool arg_starts_with_name = false) noexcept; + static bool set_category (const char *name, const char *arg, size_t arg_length, unsigned int entry, bool arg_starts_with_name = false) noexcept; private: static inline LogTimingCategories _log_timing_categories; diff --git a/src/native/clr/runtime-base/logger.cc b/src/native/clr/runtime-base/logger.cc index 9a1d04bee73..5f476d25c59 100644 --- a/src/native/clr/runtime-base/logger.cc +++ b/src/native/clr/runtime-base/logger.cc @@ -85,6 +85,11 @@ auto Logger::open_file (LogCategories category, const char *custom_path, const c if (ret != nullptr) { return log_and_return (ret, custom_path); } + + if (override_dir == nullptr || *override_dir == '\0') { + return nullptr; + } + Util::create_public_directory (override_dir); char stack_buffer [Util::LocalPathBufferSize]; char *path_buffer = Util::join_paths (stack_buffer, sizeof (stack_buffer), override_dir, fallback_filename); diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 38ec215d561..3db6f3505cb 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -175,9 +175,12 @@ AndroidSystem::monodroid_get_system_property (const char *name, dynamic_local_st const char* AndroidSystem::monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept { - if (value == nullptr || value_size < PROPERTY_VALUE_BUFFER_LEN) { - return nullptr; - } + // `__system_property_get` always writes up to `PROPERTY_VALUE_BUFFER_LEN` bytes, so a smaller + // buffer would overflow. This is a programming error, not a runtime condition. + abort_unless ( + value != nullptr && value_size >= PROPERTY_VALUE_BUFFER_LEN, + "System property value buffer is too small" + ); value [0] = '\0'; From 11547a4f12f822f3efa81d3a6fc606d7ed55e037 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 18:31:15 +0200 Subject: [PATCH 24/26] [native] Correct the documented lifetime of bundled property values `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`, 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 --- src/native/clr/include/runtime-base/android-system.hh | 6 ++++-- src/native/clr/runtime-base/android-system-shared.cc | 6 +++--- src/native/mono/runtime-base/android-system.cc | 6 +++--- src/native/mono/runtime-base/android-system.hh | 7 ++++++- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index f2b2a0f76a5..43ad2a89114 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -136,8 +136,10 @@ namespace xamarin::android { // to receive Android system properties. Bundled properties are returned without copying, so // their length is not limited by `value_size`. // - // Nothing is ever allocated: the result is either `value` or a pointer to application data - // which lives as long as the process. The caller must not free it. + // Nothing is ever allocated and the caller must not free the result: it is either `value` or + // a pointer to bundled property data. The latter is only guaranteed to stay valid until the + // next `setup_environment()` call, because in Debug builds bundled properties are stored in + // a mutable map. Copy the value if you need to retain it. static auto monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept -> const char*; static void detect_embedded_dso_mode (jstring_array_wrapper& appDirs) noexcept; static void setup_environment () noexcept; diff --git a/src/native/clr/runtime-base/android-system-shared.cc b/src/native/clr/runtime-base/android-system-shared.cc index 3ad74670d25..88c455a47b2 100644 --- a/src/native/clr/runtime-base/android-system-shared.cc +++ b/src/native/clr/runtime-base/android-system-shared.cc @@ -20,9 +20,9 @@ auto AndroidSystem::monodroid_get_system_property (const char *name, char *value return value; } - // Bundled properties are NUL-terminated strings in static application data which live as long - // as the process, so return them directly rather than copying them into `value`. Their length - // is therefore not limited by `Constants::PROPERTY_VALUE_BUFFER_LEN`. + // Bundled properties are NUL-terminated strings owned by the application, so return them + // directly rather than copying them into `value`. Their length is therefore not limited by + // `Constants::PROPERTY_VALUE_BUFFER_LEN`. See the header for the exact lifetime guarantee. // // A bundled property may be present but empty. `__system_property_get` cannot distinguish an // empty value from a missing one either, so report both as unset and let callers keep their diff --git a/src/native/mono/runtime-base/android-system.cc b/src/native/mono/runtime-base/android-system.cc index 3db6f3505cb..a7994c94878 100644 --- a/src/native/mono/runtime-base/android-system.cc +++ b/src/native/mono/runtime-base/android-system.cc @@ -189,9 +189,9 @@ AndroidSystem::monodroid_get_system_property (const char *name, char *value, siz return value; } - // Bundled properties are NUL-terminated strings in static application data which live as long - // as the process, so return them directly rather than copying them into `value`. Their length - // is therefore not limited by `PROPERTY_VALUE_BUFFER_LEN`. + // Bundled properties are NUL-terminated strings owned by the application, so return them + // directly rather than copying them into `value`. Their length is therefore not limited by + // `PROPERTY_VALUE_BUFFER_LEN`. See the header for the exact lifetime guarantee. // // A bundled property may be present but empty. `__system_property_get` cannot distinguish an // empty value from a missing one either, so report both as unset and let callers keep their diff --git a/src/native/mono/runtime-base/android-system.hh b/src/native/mono/runtime-base/android-system.hh index 9299b7b5155..eec1e99fff0 100644 --- a/src/native/mono/runtime-base/android-system.hh +++ b/src/native/mono/runtime-base/android-system.hh @@ -97,11 +97,16 @@ namespace xamarin::android::internal { static int monodroid_get_system_property (const char *name, dynamic_local_string &value) noexcept; // Returns the property's NUL-terminated value, or `nullptr` if it is not set. A property // that is set to an empty value is reported as not set, matching `__system_property_get`, - // which cannot tell the two apart. The result is valid for at least as long as `value` is. + // which cannot tell the two apart. // // `value` is a scratch buffer of at least `PROPERTY_VALUE_BUFFER_LEN` bytes, used to receive // Android system properties. Bundled properties are returned without copying, so their // length is not limited by `value_size`. + // + // Nothing is ever allocated and the caller must not free the result: it is either `value` or + // a pointer to bundled property data. The latter is only guaranteed to stay valid until the + // next `add_system_property()` call, which frees the value it replaces. Copy the value if + // you need to retain it. static const char* monodroid_get_system_property (const char *name, char *value, size_t value_size) noexcept; static int monodroid_get_system_property (std::string_view const& name, char **value) noexcept From c4768da188226abae2a378cca01e7a793120d957 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 18:36:46 +0200 Subject: [PATCH 25/26] [native] Fix two latent defects found while self-reviewing * `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 --- src/native/clr/include/host/host-environment.hh | 1 + src/native/clr/include/runtime-base/android-system.hh | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/native/clr/include/host/host-environment.hh b/src/native/clr/include/host/host-environment.hh index 137beb468bd..575ac02f017 100644 --- a/src/native/clr/include/host/host-environment.hh +++ b/src/native/clr/include/host/host-environment.hh @@ -56,6 +56,7 @@ namespace xamarin::android { uint32_t const count, AppEnvironmentVariable const (&entries)[], const char (&contents)[]) noexcept -> const char* { + value_len = 0; if (count == 0) { return nullptr; } diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 43ad2a89114..0a44d3f924a 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -172,11 +172,17 @@ namespace xamarin::android { static auto format_primary_override_dir (jstring_wrapper &home, char *buffer, size_t buffer_size) noexcept -> ssize_t { abort_unless (buffer != nullptr, "Primary override directory buffer must not be null"); + + // `jstring_wrapper::get_cstr()` returns `nullptr` for a null `jstring`, and passing that + // to `%s` is undefined behaviour. An app without a files directory cannot work anyway. + const char *home_path = home.get_cstr (); + abort_unless (home_path != nullptr, "Application home directory must not be null"); + int length = snprintf ( buffer, buffer_size, "%s/%.*s/%.*s", - home.get_cstr (), + home_path, static_cast(Constants::OVERRIDE_DIRECTORY_NAME.length ()), Constants::OVERRIDE_DIRECTORY_NAME.data (), static_cast(Constants::android_lib_abi.length ()), From f8ca254a366c2ac66a2de8354253adcf6af4040d Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 28 Aug 2026 18:46:33 +0200 Subject: [PATCH 26/26] [native] Consolidate the stack-buffer-with-heap-fallback retry `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 --- src/native/clr/host/typemap.cc | 16 +++---- .../include/runtime-base/android-system.hh | 20 ++++---- src/native/clr/include/runtime-base/util.hh | 47 +++++++++++++++---- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/native/clr/host/typemap.cc b/src/native/clr/host/typemap.cc index 1743767e7f8..43c91d024c3 100644 --- a/src/native/clr/host/typemap.cc +++ b/src/native/clr/host/typemap.cc @@ -174,15 +174,13 @@ auto TypeMapper::index_to_name (ssize_t idx, const char* typeName, const TypeMap auto TypeMapper::managed_to_java_debug (const char *typeName, const char *assemblyFullName) noexcept -> const char* { char stack_buffer [Constants::SENSIBLE_PATH_MAX]; - char *full_type_name = stack_buffer; - ssize_t result = format_managed_type_name (typeName, assemblyFullName, full_type_name, sizeof (stack_buffer)); - if (result < 0) { - size_t required_capacity = static_cast(-result); - full_type_name = static_cast (std::malloc (required_capacity)); - abort_unless (full_type_name != nullptr, "Failed to allocate managed type name"); - result = format_managed_type_name (typeName, assemblyFullName, full_type_name, required_capacity); - } - abort_unless (result >= 0, "Failed to format managed type name using the required capacity"); + char *full_type_name = Util::format_with_retry ( + stack_buffer, + sizeof (stack_buffer), + [typeName, assemblyFullName](char *buffer, size_t buffer_size) noexcept { + return format_managed_type_name (typeName, assemblyFullName, buffer, buffer_size); + } + ); ssize_t idx = find_index_by_hash (full_type_name, type_map.managed_to_java, type_map_managed_type_names, MANAGED, JAVA); const char *mapped_name = index_to_name (idx, full_type_name, type_map.managed_to_java, type_map_java_type_names, MANAGED, JAVA); diff --git a/src/native/clr/include/runtime-base/android-system.hh b/src/native/clr/include/runtime-base/android-system.hh index 0a44d3f924a..57ac8d6bbdd 100644 --- a/src/native/clr/include/runtime-base/android-system.hh +++ b/src/native/clr/include/runtime-base/android-system.hh @@ -201,17 +201,17 @@ namespace xamarin::android { static auto determine_primary_override_dir (jstring_wrapper &home) noexcept -> std::string { char stack_buffer [Constants::SENSIBLE_PATH_MAX]; - char *name = stack_buffer; - ssize_t result = format_primary_override_dir (home, name, sizeof (stack_buffer)); - if (result < 0) { - size_t required_capacity = static_cast(-result); - name = static_cast (std::malloc (required_capacity)); - abort_unless (name != nullptr, "Failed to allocate primary override directory path"); - result = format_primary_override_dir (home, name, required_capacity); - } - abort_unless (result >= 0, "Failed to format primary override directory path using the required capacity"); + size_t length; + char *name = Util::format_with_retry ( + stack_buffer, + sizeof (stack_buffer), + [&home](char *buffer, size_t buffer_size) noexcept { + return format_primary_override_dir (home, buffer, buffer_size); + }, + &length + ); - std::string path { name, static_cast(result) }; + std::string path { name, length }; if (name != stack_buffer) { std::free (name); } diff --git a/src/native/clr/include/runtime-base/util.hh b/src/native/clr/include/runtime-base/util.hh index 22cef78e193..6d7472aaf07 100644 --- a/src/native/clr/include/runtime-base/util.hh +++ b/src/native/clr/include/runtime-base/util.hh @@ -344,19 +344,46 @@ namespace xamarin::android { return static_cast(path_length); } - static auto join_paths (char *stack_buffer, size_t stack_buffer_size, std::string_view first, std::string_view second) noexcept -> char* + // Formats a string that usually fits in a stack buffer, falling back to the heap when it + // does not. `formatter` must write into the buffer it is given and return the formatted + // length excluding the terminating NUL, or the negative required capacity including it — + // the protocol implemented by `format_joined_path()` and friends. + // + // Returns `stack_buffer`, or a heap block the caller must `std::free ()`. Compare the + // result against `stack_buffer` to tell the two apart. When `length` is not `nullptr`, it + // receives the formatted length excluding the terminating NUL. + template + static auto format_with_retry (char *stack_buffer, size_t stack_buffer_size, TFormatter formatter, size_t *length = nullptr) noexcept -> char* { - ssize_t result = format_joined_path (stack_buffer, stack_buffer_size, first, second); - if (result >= 0) { - return stack_buffer; + ssize_t result = formatter (stack_buffer, stack_buffer_size); + if (result < 0) { + size_t required_capacity = static_cast(-result); + char *heap_buffer = static_cast (std::malloc (required_capacity)); + abort_unless (heap_buffer != nullptr, "Failed to allocate formatted string"); + + result = formatter (heap_buffer, required_capacity); + abort_unless (result >= 0, "Failed to format string using the required capacity"); + if (length != nullptr) { + *length = static_cast(result); + } + return heap_buffer; } - size_t required_capacity = static_cast(-result); - char *heap_buffer = static_cast (std::malloc (required_capacity)); - abort_unless (heap_buffer != nullptr, "Failed to allocate joined path"); - result = format_joined_path (heap_buffer, required_capacity, first, second); - abort_unless (result >= 0, "Failed to join path using the required capacity"); - return heap_buffer; + if (length != nullptr) { + *length = static_cast(result); + } + return stack_buffer; + } + + static auto join_paths (char *stack_buffer, size_t stack_buffer_size, std::string_view first, std::string_view second) noexcept -> char* + { + return format_with_retry ( + stack_buffer, + stack_buffer_size, + [first, second](char *buffer, size_t buffer_size) noexcept { + return format_joined_path (buffer, buffer_size, first, second); + } + ); } private: