Fix JNI string lifetime/null-safety, Windows cancel-drain race, and UWP version parse#1494
Open
bmehta001 wants to merge 25 commits into
Open
Fix JNI string lifetime/null-safety, Windows cancel-drain race, and UWP version parse#1494bmehta001 wants to merge 25 commits into
bmehta001 wants to merge 25 commits into
Conversation
…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>
lalitb
reviewed
Jun 22, 2026
lalitb
reviewed
Jun 22, 2026
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>
Contributor
There was a problem hiding this comment.
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_sendSignaland add null checks forGetStringUTFCharsfailures before constructingstd::string. - Remove a Windows HTTP cancel/drain data race by checking
m_requests.empty()under the same mutex used byerase(). - Guard UWP
DeviceFamilyVersionparsing (std::stoull) with exception handling and preserve the existingversionDec == 0fallback 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.
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>
…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'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>
…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>
…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>
…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>
This change adds a std::condition_variable_any member; <condition_variable> is the only include it needs. Drop the <mutex> include: the pre-existing recursive_mutex member already resolves via the transitively-included pal/PAL.hpp, so adding <mutex> addressed a pre-existing concern outside this change's scope. 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>
…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>
…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>
…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>
…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>
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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles four small, latent safety fixes from a repo-wide material-bug review. None are recent regressions — they were introduced between 2018 and 2023.
1.
Signals_jni.cpp— use-after-free + pending JNI exception (🟠)sendSignalcalledReleaseStringUTFCharson the UTF buffer before passing it toSignals::CreateEventProperties(which copies from it by value), reading freed/unpinned memory. Moved the release to after the buffer is consumed.Also, when
GetStringUTFCharsreturnsnull(e.g. OOM) it leaves a pending Java exception;sendSignalandnativeInitializereturnedfalsewithout clearing it, so the JVM would throw that exception at the Java call site instead of the caller observing the intended gracefulfalse. Both sites now callenv->ExceptionClear()before returning, so a failed telemetry signal degrades cleanly rather than propagating an OOM into the host app.2. HTTP cancel-drain: data race + unbounded spin/hang (🟡🟠) — Fixes #1437
The
CancelAllRequests()drain loops readm_requests.empty()with no lock held, whileerase()mutates thestd::mapon the WinInet callback / PPL continuation thread underm_requestsMutex— a data race / UB. Now read under the lock, mirroring the already-correct WinRt destructor drain. (#1460 fixed the destructor's drain but not these methods.)Beyond the race, both
HttpClientManager::cancelAllRequests()andHttpClient_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, freezingLogEvent. Replaced both poll loops with a condition variable signaled from the drain site (onHttpResponse/erase), bounded by a timeout safety valve: the CV drains the common case in microseconds, and the timeout guarantees the drain can never spin or block indefinitely (so the LogManager lock is held for at most the timeout, not forever).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. 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 deref (🟢)GetStringUTFCharsreturnsnullon allocation failure; the result was fed straight into astd::stringconstructor. Added a null check before constructing.4.
WindowsRuntimeSystemInformationImpl.cpp— UWP parse (🟢)std::stoullon theDeviceFamilyVersionstring was unguarded (would throw on malformed/oversized input). Guarded with try/catch defaulting to0(the code already has aversionDec == 0→"10.0"fallback).Validation
Signals_jni(needs the privatesignalsmodule) can't be built on the dev host; its exact JNI call sequence (GetStringUTFChars+ExceptionClear+ReleaseStringUTFChars) plusJniConvertors.cpp/OfflineStorage_Room.cpppass NDKaarch64 -fsyntax-onlyunder-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); a unit test
HttpClientManagerTestsholds a request open and assertscancelAllRequests()returns within the (shortened) timeout instead of hanging, then completes the outstanding callback to confirm it stays safe after the drain is abandoned; andHttpClient_WinInet.cppcompiles cleanly under MSVC/permissive- /W4.