Fix JNI string lifetime/null-safety, Windows cancel-drain race, and UWP version parse by bmehta001 · Pull Request #1494 · microsoft/cpp_client_telemetry

bmehta001 · 2026-06-22T16:53:37Z

Bundles four small, latent safety fixes from a repo-wide material-bug review.

1. `Signals_jni.cpp` — use-after-free + null/pending-JNI-exception handling (🟠)

sendSignal called ReleaseStringUTFChars on the UTF buffer before passing it to Signals::CreateEventProperties (which copies from it by value), reading freed/unpinned memory. Moved the release to after the buffer is consumed.

Both entrypoints now also harden their JNI string reads: sendSignal returns false on null signal_item_json, and both sendSignal / nativeInitialize clear the pending Java exception and return false when GetStringUTFChars returns null (e.g. OOM) instead of dereferencing/passing along the null buffer. nativeInitialize likewise only reads/releases base_url when the Java string itself is non-null, so failed telemetry setup degrades cleanly rather than propagating an OOM or crashing the host app.

2. HTTP cancel-drain: data race + unbounded spin/hang (🟡🟠) — Fixes #1437

HttpClient_WinInet::CancelAllRequests() and HttpClient_WinRt::CancelAllRequests() read m_requests.empty() with no lock held, while erase() mutates the request map under m_requestsMutex on the WinInet callback / PPL continuation thread — a data race / UB. Both now re-check emptiness under the lock; WinInet additionally wakes waiters from erase() via a condition variable. (#1460 fixed the destructor's drain but not these methods.)

Beyond the race, HttpClientManager::cancelAllRequests() and HttpClient_WinInet::CancelAllRequests() drained with an unbounded poll loop — the root of #1437 (a macOS spindump). If the drainer never runs (the SDK task dispatcher stalls/stops, or a WinInet callback stalls), the loop blocks forever while holding the LogManager lock, freezing LogEvent. Those two paths now drain via a condition variable signaled from onHttpResponse / erase(); HttpClient_WinRt::CancelAllRequests() stays poll-based but now caps best-effort waits to the remaining deadline so it cannot run indefinitely or overshoot the timeout by a full poll interval.

The drain distinguishes best-effort from full-drain: pause-style callers (which may hold the LogManager lock and where the manager is not being destroyed) drain best-effort and bounded, so outstanding callbacks stay valid and are invoked later; shutdown/teardown callers do a full drain barrier. HttpClientManager uses a new IBoundedHttpClientCancel capability to pass the same deadline into synchronous Windows transports (WinInet / WinRt) without changing the public IHttpClient::CancelAllRequests() shape, and TelemetrySystem::pause() opts into that best-effort path. Callbacks are deliberately not force-cleared on timeout — the HTTP client still owns them and invokes them later, so deleting them would use-after-free.

3. `JniConvertors.cpp` / `OfflineStorage_Room.cpp` — null JNI string reads (🟢)

GetStringUTFChars returns null on allocation failure; several Room/JNI paths fed that straight into a std::string / StorageRecord construction or unconditionally released the result. JStringToStdString now clears the pending JNI exception and returns ""; OfflineStorage_Room.cpp now tolerates null tenant strings, avoids constructing from / releasing null, and skips recording a dropped-record entry when the tenant token read itself failed rather than misattributing it to an empty token.

In GetRecords, the guarded tenant-token read now calls ThrowRuntime immediately afterward so a failed GetStringUTFChars does not leave a pending JNI exception in flight while later JNI calls continue.

4. `WindowsRuntimeSystemInformationImpl.cpp` — UWP parse (🟢)

std::stoull on the DeviceFamilyVersion string was unguarded (would throw on malformed/oversized input). Guarded with try/catch defaulting to 0 (the code already has a versionDec == 0 → "10.0" fallback).

Validation

Signals_jni (needs the private signals module) can't be built on the dev host; its exact JNI call sequence (GetStringUTFChars + ExceptionClear + ReleaseStringUTFChars) plus JniConvertors.cpp / OfflineStorage_Room.cpp pass NDK aarch64 -fsyntax-only under -Wall -Werror; the UWP path relies on Windows CI.

The cancel-drain fix (#2) is verified: the full FuncTests suite (39 tests) passes on Linux (CV drain, no regression); unit tests in HttpClientManagerTests now cover both behaviors — CancelAllRequests_TimesOutInsteadOfHanging holds a request open and asserts best-effort cancelAllRequests() returns within the (shortened) timeout instead of hanging, then completes the outstanding callback to confirm it stays safe after the drain is abandoned; CancelAllRequests_UsesBoundedCancelCapability asserts the bounded Windows-capability path is used instead of per-request async fallback; and HttpClient_WinInet.cpp compiles cleanly under MSVC /permissive- /W4.

…WP 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 microsoft#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/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>

Copilot

Pull request overview

This PR bundles several small safety fixes across the Android JNI layer and Windows-specific implementations to eliminate latent undefined behavior (use-after-free / data race), improve null-safety around JNI string conversion, and harden UWP OS version parsing against malformed input.

Changes:

Fix JNI string lifetime in Signals_sendSignal and add null checks for GetStringUTFChars failures before constructing std::string.
Remove a Windows HTTP cancel/drain data race by checking m_requests.empty() under the same mutex used by erase().
Guard UWP DeviceFamilyVersion parsing (std::stoull) with exception handling and preserve the existing versionDec == 0 fallback behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
lib/jni/Signals_jni.cpp	Prevents using a UTF buffer after it has been released; adds null-safety for null `jstring` / failed UTF conversion.
lib/jni/JniConvertors.cpp	Avoids constructing `std::string` from a null UTF pointer when `GetStringUTFChars` fails.
lib/offline/OfflineStorage_Room.cpp	Adds a null check before converting a UTF pointer to `std::string`, preventing a null deref / misattribution.
lib/http/HttpClient_WinRt.cpp	Fixes a drain-loop data race by reading `m_requests.empty()` under `m_requestsMutex`.
lib/http/HttpClient_WinInet.cpp	Fixes a drain-loop data race by reading `m_requests.empty()` under `m_requestsMutex`.
lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp	Hardens UWP version parsing by catching `std::stoull` exceptions and falling back to `0`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lalitb

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>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

…ot review) Unlike the other JNI string reads in this file, the GetRecords() path called env->GetStringUTFChars(tenant_j, ...) with neither a pre-call null check on the jstring nor a following ThrowRuntime() exception check. Passing a null jstring to GetStringUTFChars is undefined per the JNI spec (and can leave a pending NullPointerException in flight). Only call GetStringUTFChars when tenant_j is non-null; the downstream already tolerates a null tenant_utf (defaults to "" and skips the guarded ReleaseStringUTFChars). Validated: NDK 27 clang++ --target=aarch64-linux-android24 -fsyntax-only compiles OfflineStorage_Room.cpp (USE_ROOM) cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Copilot's re-review flagged two more JNI string reads where the result was null-checked but the jstring input was passed to GetStringUTFChars unguarded, which crashes (or leaves a pending exception) before the result check when the jstring is null: - Signals_jni.cpp nativeInitialize: only read base_url when non-null; if the read itself fails (e.g. OOM) return immediately rather than continuing JNI calls with an exception pending, matching the nativeLog(signal_item_json) site. - OfflineStorage_Room.cpp ByTenant releaseRecords (token) and GetRecords-by-tenant (tenantToken_java): only call GetStringUTFChars when the jstring is non-null; the downstream already tolerates a null result (skips/defaults). The getSetting site is already guarded by an enclosing if (java_value). Validated: NDK 27 clang++ --target=aarch64-linux-android24 -fsyntax-only compiles both files cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

…lot review) If GetStringUTFChars(tenant_j) fails (e.g. OOM) it returns null and leaves a pending JNI exception. GetRecords() previously continued issuing Get*Field calls with that exception in flight, which can make them return defaults. Call ThrowRuntime after the read (as GetAndReserveRecords and the rest of this file already do) to describe+clear the exception, notify the observer, and unwind the batch via the surrounding try/catch. Validated: NDK 27 clang++ -fsyntax-only compiles cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

…ot review) When GetStringUTFChars returns null (e.g. OOM) it leaves a pending Java exception. JStringToStdString returned "" without clearing it, so callers kept making JNI calls with an exception in flight. Clear the pending exception before returning, consistent with the ExceptionClear handling elsewhere in the JNI layer. Keeps the existing string-returning contract rather than redesigning the helper's signature, so no call sites change. Validated: NDK 27 clang++ -fsyntax-only compiles cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

…eview) The DeviceFamilyVersion parse added a catch(const std::exception&) but the file only pulled in <exception> transitively. Include it explicitly so the build does not depend on transitive include order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The header uses std::recursive_mutex directly, so it should include <mutex> rather than rely on it being pulled in transitively via pal/PAL.hpp. This keeps the header self-contained (include-what-you-use) alongside <condition_variable> for the condition_variable_any member. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

…orms The bestEffort drain in cancelAllRequests caps the wait on m_httpCallbacks, but on Windows (USE_SYNC_HTTPRESPONSE_HANDLER) onHttpResponse drains m_httpCallbacks synchronously inside m_httpClient.CancelAllRequests(), so that wait is usually already satisfied and the real blocking is the transport-level wait there (WinInet condition-variable wait, WinRt poll), which is not bounded. Correct the comment so it no longer implies pause is fully bounded on Windows; fully bounding it requires plumbing the deadline into IHttpClient::CancelAllRequests (follow-up). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

…AllRequests PauseTransmission could block indefinitely on Windows even with the manager-level drain cap: with USE_SYNC_HTTPRESPONSE_HANDLER the callbacks drain synchronously inside IHttpClient::CancelAllRequests, so the real blocking region is the transport wait (WinInet condition-variable wait, WinRt poll), which was unbounded. Add a bestEffortTimeout parameter to IHttpClient::CancelAllRequests (default zero = full drain for shutdown/destructor; positive = best-effort cap). WinInet and WinRt now bound their drain wait by it; HttpClientManager passes m_cancelDrainTimeout on pause and zero on teardown. Fire-and-forget impls (Apple, CAPI, Android) accept and ignore the parameter. Files: lib/include/public/IHttpClient.hpp, lib/http/HttpClient_WinInet.{hpp,cpp}, HttpClient_WinRt.{hpp,cpp}, HttpClient_Apple.{hpp,mm}, HttpClient_CAPI.{hpp,cpp}, HttpClient_Android.{hpp,cpp}, HttpClientManager.{hpp,cpp}, tests/unittests/HttpClientCAPITests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

…ten the pause bound - Keep the legacy no-arg CancelAllRequests() virtual and add the timed overload as a separate method whose default forwards to it, so existing IHttpClient consumers that override CancelAllRequests() keep working (source-compatible) while built-in clients override the timed variant. - Treat m_cancelDrainTimeout as the total pause budget: subtract time already spent in the transport-level cancel before waiting on the manager callback drain, so the pause path holds the LogManager lock for at most ~one timeout, not up to 2x. - WinRt bounded poll now sleeps at most the remaining budget (min(100ms, remaining)) instead of a full 100ms, so it does not overshoot bestEffortTimeout by a poll interval. - Relax the manager timeout test's lower bound (Ge(100) for a 150ms timeout) so it still catches immediate-return regressions without being flaky under CI timer jitter. Files: lib/include/public/IHttpClient.hpp, lib/http/HttpClientManager.cpp, HttpClient_WinRt.cpp, tests/unittests/HttpClientManagerTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

…table The overload preserves source compatibility for existing IHttpClient overrides, but adding a virtual changes the vtable, so binary implementations must be recompiled -- consistent with the SDK's C++ classes not being ABI-stable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

With the legacy no-arg virtual restored on IHttpClient, an impl that overrode only the timed overload would let a CancelAllRequests() call through an IHttpClient reference hit the base no-op instead of cancelling. Each built-in client (WinInet, WinRt, Apple, CAPI, Android) now also overrides CancelAllRequests() to forward to the timed overload with a zero (full-drain) timeout, so both signatures cancel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Move bounded cancel support behind an internal optional capability and use per-request cancellation as the manager fallback for old-style IHttpClient implementations. Keep shutdown on the existing full-drain CancelAllRequests path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Fail Signals.sendSignal cleanly when Java passes a null native logger pointer, matching the existing JNI hardening in this file and avoiding a null ILogger dereference before LogEvent(). Also make HttpClientManager::cancelAllRequestsAsync() return void since every caller ignored the bool and the helper has no meaningful failure signal. Deliberately defer the WinRt cancel-drain CV conversion: the current best-effort path is already bounded, and changing the UWP/PPL completion-drain coordination without a transport-specific validation loop is riskier than this follow-up warrants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41

Sync the local worktree with the latest remote PR branch so the JNI/null-safety follow-up can be pushed without rewriting branch history. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41

bmehta001 requested a review from a team as a code owner June 22, 2026 16:53

lalitb reviewed Jun 22, 2026

View reviewed changes

Comment thread lib/jni/Signals_jni.cpp

lalitb reviewed Jun 22, 2026

View reviewed changes

Comment thread lib/offline/OfflineStorage_Room.cpp Outdated

bmehta001 requested a review from Copilot June 22, 2026 21:28

Copilot started reviewing on behalf of bmehta001 June 22, 2026 21:28 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

bmehta001 requested a review from Copilot June 24, 2026 16:58

Copilot started reviewing on behalf of bmehta001 June 24, 2026 16:59 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread lib/jni/Signals_jni.cpp Outdated

Comment thread lib/offline/OfflineStorage_Room.cpp

bmehta001 and others added 2 commits June 24, 2026 12:36

Merge branch 'main' into bhamehta/fix-jni-windows-uwp-safety

778a2e5

bmehta001 requested a review from Copilot June 25, 2026 14:50

Copilot started reviewing on behalf of bmehta001 June 25, 2026 14:51 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread lib/jni/Signals_jni.cpp

Comment thread lib/offline/OfflineStorage_Room.cpp Outdated

bmehta001 requested a review from Copilot June 25, 2026 15:13

Copilot started reviewing on behalf of bmehta001 June 25, 2026 15:14 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread lib/offline/OfflineStorage_Room.cpp

Comment thread lib/offline/OfflineStorage_Room.cpp

bmehta001 requested a review from Copilot June 25, 2026 15:26

Copilot started reviewing on behalf of bmehta001 June 25, 2026 15:27 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread lib/jni/JniConvertors.cpp

bmehta001 requested a review from Copilot June 25, 2026 15:37

Copilot started reviewing on behalf of bmehta001 June 25, 2026 15:37 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread lib/jni/Signals_jni.cpp

Comment thread lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp

bmehta001 and others added 2 commits July 9, 2026 12:05

Merge branch 'main' into bhamehta/fix-jni-windows-uwp-safety

377e70e

bmehta001 self-assigned this Jul 9, 2026

bmehta001 requested a review from Copilot July 10, 2026 16:01

Copilot started reviewing on behalf of bmehta001 July 10, 2026 16:02 View session