From cd2f1a954b0c944b0708d50312dc8a5d12de645f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 11:52:58 -0500 Subject: [PATCH 1/3] Fix four latent safety bugs (JNI lifetime/null-safety, cancel race, UWP parse) Bundled small fixes from a repo-wide review. None are recent regressions (introduced 2018-2023). 1) Signals_jni.cpp (UAF): sendSignal called ReleaseStringUTFChars on the UTF buffer *before* passing it to Signals::CreateEventProperties (which copies from it by value), reading freed/unpinned memory. Move the release to after the buffer is consumed. 2) HttpClient_WinInet.cpp / HttpClient_WinRt.cpp (data race): the CancelAllRequests() drain loop read `m_requests.empty()` with no lock held, while erase() mutates the map on the HTTP callback / PPL continuation thread under m_requestsMutex -- a data race / UB on std::map. Read empty() under the lock each iteration, mirroring the already-correct WinRt destructor drain. (The recent #1460 fixed the destructor's drain but not these methods.) 3) JniConvertors.cpp / OfflineStorage_Room.cpp (null deref): the result of GetStringUTFChars (which returns null on allocation failure) was fed straight into a std::string ctor. Null-check before constructing. 4) WindowsRuntimeSystemInformationImpl.cpp (UWP): std::stoull on the DeviceFamilyVersion string was unguarded; guard it with try/catch defaulting to 0 (the code already has a versionDec==0 -> "10.0" fallback). Validation: JniConvertors.cpp and OfflineStorage_Room.cpp pass NDK aarch64 -fsyntax-only. Signals_jni (needs the private signals module), the Windows HTTP clients, and the UWP path can't be built on this host and rely on the Android/Windows CI; the WinInet/WinRt fix mirrors the existing correct destructor pattern in the same files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinInet.cpp | 14 ++++++++++++-- lib/http/HttpClient_WinRt.cpp | 14 ++++++++++++-- lib/jni/JniConvertors.cpp | 2 ++ lib/jni/Signals_jni.cpp | 2 +- lib/offline/OfflineStorage_Room.cpp | 5 +++-- .../WindowsRuntimeSystemInformationImpl.cpp | 10 +++++++++- 6 files changed, 39 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index eaefb2318..9c920682d 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -534,11 +534,21 @@ void HttpClient_WinInet::CancelAllRequests() for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // wait for all destructors to run. Read m_requests under the lock each + // iteration; erase() runs on the WinInet callback thread under the same lock. + bool done; + { + LOCKGUARD(m_requestsMutex); + done = m_requests.empty(); + } + while (!done) { PAL::sleep(100); std::this_thread::yield(); + { + LOCKGUARD(m_requestsMutex); + done = m_requests.empty(); + } } } diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index e46e4c49c..db14aa1da 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -351,11 +351,21 @@ namespace MAT_NS_BEGIN { for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // wait for all destructors to run. Read m_requests under the lock each + // iteration; erase() runs on the PPL continuation thread under the same lock. + bool done; + { + std::lock_guard lock(m_requestsMutex); + done = m_requests.empty(); + } + while (!done) { PAL::sleep(100); std::this_thread::yield(); + { + std::lock_guard lock(m_requestsMutex); + done = m_requests.empty(); + } } }; diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index 87b7cb15b..61eceafff 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -13,6 +13,8 @@ std::string JStringToStdString(JNIEnv* env, const jstring& jstr) { size_t jstr_length = env->GetStringUTFLength(jstr); auto jstr_utf = env->GetStringUTFChars(jstr, nullptr); + if (jstr_utf == nullptr) + return ""; std::string str(jstr_utf, jstr_utf + jstr_length); env->ReleaseStringUTFChars(jstr, jstr_utf); return str; diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 59ca6f32d..8b2ef0111 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -26,10 +26,10 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env, jstring signal_item_json) { jboolean isCopy = true; const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); - env->ReleaseStringUTFChars(signal_item_json, signalItemJson); auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); + env->ReleaseStringUTFChars(signal_item_json, signalItemJson); logger->LogEvent(eventProperties); return true; } diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index 72a04d0ed..c3b61d989 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -722,8 +722,9 @@ namespace MAT_NS_BEGIN auto count = env->GetLongField(byTenant, count_id); ThrowLogic(env, "Exception fetching count"); auto utf = env->GetStringUTFChars(token, nullptr); - std::string key(utf); - env->ReleaseStringUTFChars(token, utf); + std::string key(utf != nullptr ? utf : ""); + if (utf != nullptr) + env->ReleaseStringUTFChars(token, utf); dropped[key] = static_cast(count); env.popLocalFrame(); } diff --git a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp index 2ae7e9af4..8c4ff7f66 100644 --- a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp +++ b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp @@ -80,7 +80,15 @@ namespace PAL_NS_BEGIN { // The DeviceFamilyVersion is a decimalized form of the ULONGLONG hex form. For example: // 2814750430068736 = 000A000027840000 = 10.0.10116.0 - auto versionDec = std::stoull(AnalyticsInfo::VersionInfo->DeviceFamilyVersion->Data()); + unsigned long long versionDec = 0ull; + try + { + versionDec = std::stoull(AnalyticsInfo::VersionInfo->DeviceFamilyVersion->Data()); + } + catch (const std::exception&) + { + versionDec = 0ull; + } if (versionDec != 0ull) { m_os_major_version = std::to_string(versionDec >> 16 * 3) + "." + std::to_string(versionDec >> 16 * 2 & 0xFFFF); From 4c176db78b026fbeff41a6e39be94faced8a8cd6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 16:00:31 -0500 Subject: [PATCH 2/3] Address review comments: harden two JNI string reads lib/jni/Signals_jni.cpp (sendSignal): guard the jstring argument and the GetStringUTFChars() result for null before use. A null return (e.g. OOM, with a pending exception) previously flowed into CreateEventProperties()/Release as a null pointer. Now returns false instead. lib/offline/OfflineStorage_Room.cpp (ReleaseRecords): a failed tenant-token string read was turned into an empty std::string and reported as dropped[""] = count, silently misattributing dropped records to an empty tenant token. Now follows the file's established pattern (GetStringUTFChars + ThrowRuntime) and skips the entry when the read fails instead of fabricating an empty token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 8 ++++++++ lib/offline/OfflineStorage_Room.cpp | 9 +++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 8b2ef0111..6de25c4fb 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -25,7 +25,15 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env, jlong nativeLoggerPtr, jstring signal_item_json) { jboolean isCopy = true; + if (signal_item_json == nullptr) { + return false; + } const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); + if (signalItemJson == nullptr) { + // GetStringUTFChars returned null (e.g. OOM, with a pending exception); + // there is nothing valid to log. + return false; + } auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index c3b61d989..ef4f90c63 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -722,10 +722,15 @@ namespace MAT_NS_BEGIN auto count = env->GetLongField(byTenant, count_id); ThrowLogic(env, "Exception fetching count"); auto utf = env->GetStringUTFChars(token, nullptr); - std::string key(utf != nullptr ? utf : ""); + ThrowRuntime(env, "Exception fetching token string"); + // Skip rather than misattribute dropped records to an empty + // tenant token when the string read fails. if (utf != nullptr) + { + std::string key(utf); env->ReleaseStringUTFChars(token, utf); - dropped[key] = static_cast(count); + dropped[key] = static_cast(count); + } env.popLocalFrame(); } m_observer->OnStorageRecordsDropped(dropped); From dca30a733ba7a559fa97317e737be6ea74523afb Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 24 Jun 2026 11:58:10 -0500 Subject: [PATCH 3/3] Guard all remaining GetStringUTFChars reads against null Addresses @lalitb's review: Signals_jni nativeInitialize called strlen(convertedValue) (and ReleaseStringUTFChars) on the result of GetStringUTFChars(base_url, ...) with no null check, so a failed conversion (e.g. pending OOM) would dereference null. Guard it: only read/release when non-null; an empty/failed base_url keeps the existing default (BaseUrl left unset). While here, make the JNI string null-safety consistent across the file this PR already hardens. ThrowRuntime/ThrowLogic only throw when s_throwExceptions is true; otherwise they ExceptionClear() and execution continues, so the existing checks are not sufficient on their own. The other GetStringUTFChars sites in OfflineStorage_Room.cpp still used the pointer unguarded: - GetReservedRecords: token_utf passed straight into StorageRecord and ReleaseStringUTFChars (null -> std::string(nullptr) UB / release of null). - GetSetting: result = utf with no null check. - GetRecords: tenant_utf passed into emplace_back / released unguarded. Each now mirrors the guard already added for the dropped-records path: substitute an empty token (matching JniConvertors' canonical return "" on null) and only release when non-null. This avoids null deref and release-of-null without silently changing behavior on success. Validated with NDK r29 clang -fsyntax-only (aarch64-linux-android23, -std=c++14, JNI build defines) on both files: clean. Files changed: - lib/jni/Signals_jni.cpp: null-guard base_url GetStringUTFChars - lib/offline/OfflineStorage_Room.cpp: null-guard token_utf, result, tenant_utf Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 8 +++++--- lib/offline/OfflineStorage_Room.cpp | 21 +++++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 6de25c4fb..918a84774 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -63,10 +63,12 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl jboolean isCopy = true; const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); - if (strlen(convertedValue) > 0) { - config.ServiceRequestConfig.BaseUrl = convertedValue; + if (convertedValue != nullptr) { + if (strlen(convertedValue) > 0) { + config.ServiceRequestConfig.BaseUrl = convertedValue; + } + env->ReleaseStringUTFChars(base_url, convertedValue); } - env->ReleaseStringUTFChars(base_url, convertedValue); config.ServiceRequestConfig.TimeoutMs = reinterpret_cast(timeout_ms); config.ServiceRequestConfig.RetryTimes = reinterpret_cast(retry_times); diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index ef4f90c63..dbe68ea1a 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -498,14 +498,17 @@ namespace MAT_NS_BEGIN uint8_t* end = start + env->GetArrayLength(blob_java); StorageRecord dest( std::to_string(id_java), - token_utf, + token_utf != nullptr ? token_utf : "", latency, persistence, timestamp, StorageBlob(start, end), retryCount, reservedUntil); - env->ReleaseStringUTFChars(tenantToken_java, token_utf); + if (token_utf != nullptr) + { + env->ReleaseStringUTFChars(tenantToken_java, token_utf); + } env->ReleaseByteArrayElements(blob_java, reinterpret_cast(start), 0); env.popLocalFrame(); @@ -1036,8 +1039,11 @@ namespace MAT_NS_BEGIN { auto utf = env->GetStringUTFChars(java_value, nullptr); ThrowRuntime(env, "copy setting value"); - result = utf; - env->ReleaseStringUTFChars(java_value, utf); + if (utf != nullptr) + { + result = utf; + env->ReleaseStringUTFChars(java_value, utf); + } } return result; } @@ -1277,14 +1283,17 @@ namespace MAT_NS_BEGIN auto blob_end = blob_store + blob_length; records.emplace_back( std::to_string(id_j), - tenant_utf, + tenant_utf != nullptr ? tenant_utf : "", latency, persistence, timestamp, StorageBlob(blob_store, blob_end), retryCount, reservedUntil); - env->ReleaseStringUTFChars(tenant_j, tenant_utf); + if (tenant_utf != nullptr) + { + env->ReleaseStringUTFChars(tenant_j, tenant_utf); + } env->ReleaseByteArrayElements(blob_j, elements, 0); env.popLocalFrame(); }