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

Summary

Harden JNI string lifetime and allocation-failure handling.
Fix Windows HTTP cancellation/drain races and prevent unbounded manager-side drain polling.
Make pause-time cancellation best-effort and bounded while retaining full draining for teardown.
Harden UWP device-family version parsing and Room JNI string handling.
Simplify build-tests.cmd error propagation and make its multi-process test wait for both child processes.

Cancellation and event safety

HttpClientManager now waits for callback removal using a condition variable instead of polling. Shutdown/cleanup still performs a full drain because callback state is being destroyed. Pause uses a bounded best-effort drain; if the timeout expires, outstanding callbacks are intentionally retained and may complete later.

Canceled or failed uploads continue through releaseRecords, not deleteRecords, so reserved events remain retryable. Only accepted/rejected responses delete records as before.

WinInet and WinRT implement the optional internal IBoundedHttpClientCancel capability so the transport can receive the timeout directly. Other transports use the existing per-request cancellation fallback. The WinRT implementation retains its 100 ms sleep-based compatibility polling loop; its bounded path caps each sleep to the remaining deadline. This transport-specific gap is intentional because WinRT is an uncommon legacy path and the manager-level tests cover the routing and timeout policy without requiring a WinRT runtime.

Validation

Windows CI passes x64/Win32 Debug and Release, including BasicFuncTests.sendManyRequestsAndCancel and the SQLite timing test.
HttpClientManagerTests cover bounded timeout behavior, safe completion after a best-effort timeout, and dispatch through IBoundedHttpClientCancel.
Linux, macOS, Android, iOS, package, header, documentation, and static-analysis checks were validated where available.

Notes

This PR does not change the public IHttpClient interface. IBoundedHttpClientCancel is an optional internal capability so existing transports and downstream implementations are not forced to adopt timeout-aware cancellation semantics.

…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>

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

Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837

Propagate the resolved iOS sysroot to embedding builds and keep Apple vendored targets compatible with strict warning settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837

Remove legacy Apple architecture, platform, and deployment-target inputs so standalone scripts and embedding consumers share CMAKE_OSX_* configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837

Prevent null and empty retry status arrays from reaching JNI APIs, and handle WinRT exceptions while reading the device family version. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Keep the behavior unchanged while reducing repeated rationale and centralizing the default kill-switch clock. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Return the failing build or test status instead of jumping to a self-looping end label, so CI reports failures rather than appearing hung. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…ety' into review-pr-1494-fixes

Use the tracked-request cancellation fallback for no-RTTI builds, and make the Windows test runner wait for concurrent tests and propagate every nonzero exit code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Suppressed comments (1)

lib/include/public/IHttpClient.hpp:563

The new CancelAllRequests() doc comment reads as if cancellation/drain is guaranteed, but this method’s default implementation is an empty no-op. That can mislead IHttpClient implementers and call sites; other methods in this interface explicitly mention when the default is a no-op (e.g., ApplySettings).

        /// <summary>
        /// Cancels all pending requests, draining fully before returning when the
        /// implementation owns a synchronous drain.
        /// </summary>
        virtual void CancelAllRequests() {}

Keep pending Java exceptions from GetStringUTFChars failures so OOM and related conversion errors are not silently converted into empty values or false results. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

# Conflicts: # CMakeLists.txt # build-ios.sh # build.sh # lib/CMakeLists.txt # tools/ports/cpp-client-telemetry/portfile.cmake

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

Lalit Kumar Bhasin (lalitb) reviewed Jun 22, 2026

View reviewed changes

Comment thread lib/jni/Signals_jni.cpp

Lalit Kumar Bhasin (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 requested a review from Copilot July 10, 2026 21:04

Copilot started reviewing on behalf of bmehta001 July 10, 2026 21:04 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Apple.hpp Outdated

Comment thread lib/http/HttpClient_CAPI.hpp Outdated

Comment thread lib/http/HttpClient_WinRt.hpp

Comment thread lib/http/HttpClient_WinInet.hpp

Comment thread lib/http/HttpClient_Android.hpp Outdated

bmehta001 requested a review from Copilot July 10, 2026 21:18

Copilot started reviewing on behalf of bmehta001 July 10, 2026 21:18 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

bmehta001 and others added 14 commits July 12, 2026 19:49

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

476ef4a

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

b8def81

Align vcpkg iOS deployment target

b2bd27b

Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837

Harden nullable JNI and WinRT version inputs

02c70ef

Prevent null and empty retry status arrays from reaching JNI APIs, and handle WinRT exceptions while reading the device family version. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Simplify cancellation documentation and clock setup

d7ba6b3

Keep the behavior unchanged while reducing repeated rationale and centralizing the default kill-switch clock. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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

135676d

Stop test runner looping after failures

362fd54

Return the failing build or test status instead of jumping to a self-looping end label, so CI reports failures rather than appearing hung. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Merge remote-tracking branch 'origin/bhamehta/fix-jni-windows-uwp-saf…

4b995f4

…ety' into review-pr-1494-fixes

Avoid RTTI dependency and report test failures

da833b7

Use the tracked-request cancellation fallback for no-RTTI builds, and make the Windows test runner wait for concurrent tests and propagate every nonzero exit code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

bmehta001 requested a lite review from Copilot August 7, 2026 23:50

Copilot started reviewing on behalf of bmehta001 August 7, 2026 23:51 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

bmehta001 and others added 3 commits August 7, 2026 19:06

Preserve JNI allocation failures

0a15be1

Keep pending Java exceptions from GetStringUTFChars failures so OOM and related conversion errors are not silently converted into empty values or false results. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Merge remote-tracking branch 'origin/main' into review-pr-1494-fixes

e039185

Merge remote-tracking branch 'msft/main' into pr1494-work

7cbcb30

# Conflicts: # CMakeLists.txt # build-ios.sh # build.sh # lib/CMakeLists.txt # tools/ports/cpp-client-telemetry/portfile.cmake

bmehta001 enabled auto-merge (squash) August 8, 2026 16:03

Baiju Meswani (baijumeswani) approved these changes Aug 8, 2026

View reviewed changes

bmehta001 merged commit 42cc8e5 into microsoft:main Aug 8, 2026
48 checks passed

Uh oh!

Conversation

bmehta001 commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Cancellation and event safety

Validation

Notes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmehta001 commented Jun 22, 2026 •

edited

Loading