Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Release Android X509 chain certificate GREFs - #128284

Merged
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix
May 19, 2026
Merged

Release Android X509 chain certificate GREFs#128284
simonrozsival merged 5 commits into
dotnet:mainfrom
simonrozsival:dev/simonrozsival/android-x509-gref-leak-fix

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented May 16, 2026

Copy link
Copy Markdown
Member

Fixes a JNI global-reference leak in the Android X509 chain PAL.

AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI global refs for the certificates in the built chain. Constructing X509Certificate2 from those pointers duplicates the refs for managed ownership, but the original returned refs were never released. This releases those temporary refs after the managed certificates have been created.

Regression test

BuildChainRepeatedly_DoesNotExhaustGlobalReferences is a new [OuterLoop] Android-only test that builds 8,600 6-certificate PKI chains via CertificateAuthority.BuildPrivatePki. Without this PR each successful build leaks 6 JNI global references, so 8,600 iterations would leak 51,600 — past Android's default 51,200 entry limit.

Validation

Run on a local Android emulator (API 36, arm64-v8a, Apple Silicon host, Mono interpreter):

  • With this PR's fix: ✅ passes (xharness exit 0) in ~9.5 minutes.
  • Without the fix (verified by reverting the new try/finally block in Interop.X509Chain.cs): ❌ crashes between iter ~7,750 and ~8,500 with JNI ERROR (app bug): global reference table overflow (max=51200) and a tombstone; xharness exit 80 (APP_CRASH).

CopilotAI review requested due to automatic review settings May 16, 2026 13:01
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label May 16, 2026
@simonrozsival
simonrozsival marked this pull request as draft May 16, 2026 13:03
The Android X509 chain PAL returns JNI global references for chain certificates. Creating X509Certificate2 from those pointers duplicates the references for managed ownership, leaving the native-returned references caller-owned. Release those temporary references after conversion so repeated chain builds do not exhaust ART global refs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 5853fff to cce39fdCompareMay 16, 2026 13:10
@simonrozsival
simonrozsival removed the request for review from MichalStrehovskyMay 16, 2026 13:10
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ref limit
Adds [OuterLoop] regression test
BuildChainRepeatedly_DoesNotExhaustGlobalReferences that builds a
6-certificate chain (root + 4 intermediates + endCert) 8,600 times.
Without the gref-release fix in PR dotnet#128284,
AndroidCryptoNative_X509ChainGetCertificates returns caller-owned JNI
global references that X509Certificate2 then duplicates, leaving the
native-returned refs orphaned. A 6-cert chain leaks 6 grefs per
successful build; 8,600 builds therefore leak 51,600 references, which
exceeds Android's default global-reference table limit (51,200) and
aborts the process with 'global reference table overflow (max=51200)'.
Threshold validation against the unfixed code path:
- 8,400 iterations completed successfully
- 8,500 iterations crashed with the gref-table overflow
With the managed try/finally cleanup in place
(Interop.X509Chain.X509ChainGetCertificates), 8,600 iterations complete
cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival added a commit to simonrozsival/runtime that referenced this pull request May 18, 2026
…ref limit
Adds [OuterLoop] companion test
BuildChainRepeatedlyOnFailure_DoesNotExhaustGlobalReferences that builds
a 6-certificate chain and forces it to fail (VerificationTime far in the
future, well outside cert validity) 60,000 times.
The success-path test (BuildChainRepeatedly_DoesNotExhaustGlobalReferences)
exercises X509ChainGetCertificates, which PR dotnet#128284 fixed. The failure
path is a different code path: managed code skips GetCertificates entirely
when chain.Build returns false, and instead invokes X509ChainGetErrors
and walks the resulting error list. The pal_x509chain.c side allocates
xmalloc'd UTF-16 message buffers per error; the managed side is expected
to Marshal.FreeHGlobal each one.
The failure path doesn't currently leak — the test is defensive against
future regressions that introduce a per-iteration leak on this path
(e.g., a forgotten Marshal.FreeHGlobal, a missing ReleaseGRef on a
throwable, or a regression in X509ChainBuild's exception-handling flow).
60,000 iterations is chosen to overflow the 51,200-entry JNI global
reference table even under a worst-case 1-gref-per-iteration regression.
The 6-cert chain mirrors the success-path test so the two tests cover
the same chain shape.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings May 18, 2026 13:05
@simonrozsival
simonrozsivalforce-pushed the dev/simonrozsival/android-x509-gref-leak-fix branch from 6b38047 to c36b4e7CompareMay 18, 2026 13:05

CopilotAI 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 2 out of 2 changed files in this pull request and generated 1 comment.

This was referenced May 19, 2026
@simonrozsival
simonrozsival merged commit 11b17d7 into dotnet:mainMay 19, 2026
89 of 93 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-x509-gref-leak-fix branch May 19, 2026 21:10
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/backport to release/10.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone May 21, 2026
steveisok pushed a commit that referenced this pull request May 22, 2026
Backport of #128284 to release/10.0
/cc @simonrozsival
## Customer Impact
- [x] Customer reported
- [ ] Found internally
A customer reported that their app was crashing due to a memory leak in
X509Chain class (JNI global ref table overflow).
## Regression
- [ ] Yes
- [x] No
[If yes, specify when the regression was introduced. Provide the PR or
commit if known.]
## Testing
The new unit test
(ChainTests.BuildChainRepeatedly_DoesNotExhaustGlobalReferences) passes
locally on Android emulator.
## Risk
Low. The change only affects Android and the code path is well covered
by tests.
**IMPORTANT**: If this backport is for a servicing release, please
verify that:
- For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`,
not `release/X.0`.
- For .NET 10+: The PR target branch is `release/X.0` (no `-staging`
suffix).
## Package authoring no longer needed in .NET 9
**IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet
package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older
versions.
---------
Co-authored-by: Simon Rozsival <simon@rozsival.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jkotas pushed a commit that referenced this pull request May 22, 2026
Follow-up to #128284 Follow-up to #128385 This PR removes an unnecessary long running unit test added in #128284 /cc @jkotas
simonrozsival added a commit that referenced this pull request May 29, 2026
Fixes Android X509Store PAL cleanup paths for certificate/private-key
entries and JNI local references to avoid memory leaks.
Follow-up to #128284 ## Changes
- Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by
`AndroidCertificatePal`.
- Release JNI local references on Android X509Store cleanup paths:
- trusted certificate enumeration
- default store open failure/success cleanup
- remove-certificate early success path
- Add JNI exception checks when advancing Android KeyStore alias
enumerations.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 21, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@simonrozsival@vcsjones@steveisok@bartonjs