Skip to content

[Android] Harden JNI resource ownership - #1525

Merged
bmehta001 merged 8 commits into
microsoft:mainfrom
bmehta001:bhamehta/jni-resource-safety
Aug 28, 2026
Merged

[Android] Harden JNI resource ownership#1525
bmehta001 merged 8 commits into
microsoft:mainfrom
bmehta001:bhamehta/jni-resource-safety

Conversation

@bmehta001

Copy link
Copy Markdown
Contributor

Description

Harden normal Android JNI resource ownership and failure handling found during the repository-wide follow-up to #1523.

  • Stop using JNI strings after acquisition fails, and stop common-context conversion immediately while a Java exception is pending.
  • Pass UUID context as GUID_t; the previous code converted the UUID string but accidentally passed the Java jstring, selecting the boolean SetContext overload.
  • Retain debug-event classes/listeners with checked global references, bound callback-local references, and attach/detach native callback threads correctly.
  • Fix zero-based listener identities, reuse one callback when a Java listener observes multiple event types, and release global references after its final explicit removal.
  • Keep Privacy Guard custom event-name storage alive for as long as any shared guard instance and avoid publishing partially initialized singleton state.

The listener changes follow the existing explicit lifecycle: callers remove debug listeners before closing their manager. This PR does not add support for concurrent or reentrant listener mutation.

Validation

  • Built the Android arm64 native library with NDK 29 and MATSDK_WARNINGS_AS_ERRORS=ON, exempting only the pre-existing pessimizing-move, unused-variable, and sign-compare warning categories.
  • Compiled the MAE SDK Java sources and Android instrumentation sources.
  • Assembled the debug instrumentation APK and native libraries for arm64-v8a, armeabi-v7a, x86, and x86_64.
  • Added instrumentation coverage for zero-based listener identity, callback reuse across event types, and identity release after final removal.

No Android device was connected, so the instrumentation APK was assembled but not executed.

Fix normal Android JNI paths that could retain invalid pointers, leak JNI references, dispatch UUID context through the wrong overload, or publish partially initialized Privacy Guard state.

Files changed:
- lib/jni/LogManager_jni.cpp: validate string conversion, correct UUID context, and safely own debug callbacks.
- lib/jni/JniConvertors.cpp: stop array conversion when JNI raises an exception.
- lib/jni/PrivacyGuard_jni.cpp: co-own borrowed event names and fail before singleton publication.
- lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java: reset listener identities after final removal.
- lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java: cover listener identity reuse and release.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d7d2f27a-7339-4585-ad02-9f89ce20ef40
@bmehta001
bmehta001 requested a review from a team as a code owner August 24, 2026 04:39
Pin JNI callbacks across reentrant dispatch, make add/remove registration transitions transactional, and atomically snapshot Privacy Guard state so teardown cannot race active users or commit partial JNI conversions.

Files changed:
- lib/callbacks/DebugSource.cpp: snapshot dispatch listeners and track pending callback lifetimes.
- lib/callbacks/DebugSourceInternal.hpp: expose internal pending-listener cleanup hooks.
- lib/jni/LogManager_jni.cpp: serialize listener state transitions without lock inversion and retain callbacks through dispatch.
- lib/jni/PrivacyGuard_jni.cpp: publish atomic shared snapshots and reject partial conversion results.
- tests/unittests/DebugEventSourceTests.cpp: cover reentrant removal during dispatch.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: de6b1234-574e-4b12-9f45-88ff809818ab

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens Android JNI resource ownership and failure handling across the SDK’s JNI bridge, focusing on correct lifetime management (JNI references and borrowed string storage) and safer behavior when Java exceptions are pending.

Changes:

  • Improves JNI string/array conversion paths to stop using results after conversion failures and to bail out when a Java exception is pending.
  • Reworks Privacy Guard initialization to retain event-name storage for the lifetime of the native instance and to avoid exposing partially initialized singleton state.
  • Strengthens debug-event listener lifecycle/identity handling, including listener reuse across event types and safer release behavior; adds/extends tests for snapshot semantics and identity reuse.

Reviewed changes

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

Show a summary per file
File Description
tests/unittests/DebugEventSourceTests.cpp Adds a unit test validating listener snapshot behavior when listeners remove others during dispatch.
lib/jni/PrivacyGuard_jni.cpp Makes PrivacyGuard initialization exception-safe and fixes lifetime of borrowed event-name strings via shared state + atomic load/store.
lib/jni/LogManager_jni.cpp Hardens JNI string conversions and significantly reworks debug event listener registration/removal and callback lifetime management.
lib/jni/JniConvertors.cpp Adds exception checks during JNI array-to-vector conversion to stop on failure and clean up local refs correctly.
lib/callbacks/DebugSourceInternal.hpp Introduces internal API for tracking “pending” listeners during dispatch and a pending-release callback hook.
lib/callbacks/DebugSource.cpp Implements pending-listener tracking to keep dispatch snapshots valid under re-entrant listener mutation.
lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java Updates native remove-listener API to return updated identity and pass the listener instance for validation.
lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java Extends instrumentation coverage for listener identity reuse across event types and final identity release after removal.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/jni/LogManager_jni.cpp Outdated
Avoid cross-translation-unit static initialization ordering by registering the callback once from nativeAddEventListener.

Files changed:
- lib/jni/LogManager_jni.cpp: replace eager registration with std::call_once.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7e12a191-38f1-48e1-baea-bdfc47d23030

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

lib/jni/JniConvertors.cpp:212

  • If GetObjectArrayElement triggers a Java exception, the code returns early without ensuring any non-null local ref returned by GetObjectArrayElement is released. This can leak a local reference in exceptional paths and contribute to LocalReferenceTable overflows in long-running JNI calls.
        auto jStringValue = static_cast<jstring>(env->GetObjectArrayElement(jArrayToConvert, i));
        if (env->ExceptionCheck())
        {
            return stringVector;
        }

Prevent a local-reference leak when GetObjectArrayElement leaves a Java exception pending.

Files changed:
- lib/jni/JniConvertors.cpp: delete the returned local reference before the exceptional return.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7e12a191-38f1-48e1-baea-bdfc47d23030
@bmehta001
bmehta001 requested a lite review from Copilot August 28, 2026 22:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

bmehta001 and others added 3 commits August 28, 2026 17:55
Call uploadAsyncParent directly so implementation tests do not trigger an uninteresting gMock call through the forwarding default action.

Files changed:
- tests/unittests/TransmissionPolicyManagerTests.cpp: use the existing parent helper in upload implementation tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7e12a191-38f1-48e1-baea-bdfc47d23030
@bmehta001
bmehta001 merged commit fc11bed into microsoft:main Aug 28, 2026
38 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants