Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,33 @@ internal static X509Certificate2[] X509ChainGetCertificates(SafeX509ChainContext
var certPtrs = new IntPtr[count];

int res = Interop.AndroidCrypto.X509ChainGetCertificates(ctx, certPtrs, certPtrs.Length);
if (res == 0)
throw new CryptographicException();

Debug.Assert(res <= certPtrs.Length);

var certs = new X509Certificate2[certPtrs.Length];
for (int i = 0; i < res; i++)
try
{
if (res == 0)
throw new CryptographicException();

Debug.Assert(res <= certPtrs.Length);

for (int i = 0; i < res; i++)
{
// X509Certificate2 duplicates these JNI global refs; the native-returned refs remain caller-owned.
certs[i] = new X509Certificate2(certPtrs[i]);
}
}
finally
{
certs[i] = new X509Certificate2(certPtrs[i]);
// The native side can populate part of certPtrs and then fail (returning 0) if a JNI
// exception is thrown mid-loop, so release every non-null entry rather than only the
// first `res` entries.
for (int i = 0; i < certPtrs.Length; i++)
{
if (certPtrs[i] != IntPtr.Zero)
{
Interop.JObjectLifetime.DeleteGlobalReference(certPtrs[i]);
}
}
}

if (res == certPtrs.Length)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public static partial class PlatformDetection

private static readonly Lazy<bool> s_IsInHelix = new Lazy<bool>(() => Environment.GetEnvironmentVariables().Keys.Cast<string>().Any(key => key.StartsWith("HELIX")));
public static bool IsInHelix => s_IsInHelix.Value;
public static bool IsNotInHelix => !IsInHelix;

public static bool IsNetCore => Environment.Version.Major >= 5 || RuntimeInformation.FrameworkDescription.StartsWith(".NET Core", StringComparison.OrdinalIgnoreCase);
public static bool IsMonoRuntime => Type.GetType("Mono.RuntimeStructs") != null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Security.Cryptography.X509Certificates.Tests.Common;
using System.Text;
using System.Threading;
using Test.Cryptography;
Expand Down Expand Up @@ -373,6 +374,83 @@ public static void BuildChainCustomTrustStore(
}
}

[PlatformSpecific(TestPlatforms.Android)]
[OuterLoop]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotInHelix))]
public static void BuildChainRepeatedly_DoesNotExhaustGlobalReferences()
{
// Android aborts the process when its JNI global reference table overflows. This
// 6-certificate chain leaks 6 JNI global refs per successful build without the Android
// PAL cleanup, so 8,600 builds would leak 51,600 certificate refs. 8,400 iterations
// completed without the fix during threshold testing, while 8,500 iterations crashed
// with "global reference table overflow (max=51200)".
// This tests runs for ~10 minutes on an Android emulator.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

10 minutes is a lot for a regression test like this. I do not think it is worth it to burn 10 minutes every (outerloop) test run for this regression test.

Are there better ways to write a regression test for this? For example, is it possible to inspect the global reference table or set it artificially small so that the process hits the leak limit much faster?

If it is not possible, I think we should fix the bug without adding a regression test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example, #128325 is fixing similar type of memory leak without a regression test and that's fine. If we were to add a test that tries to leak all memory or handles for every memory leak bug fix, our test runs would become non-sustainable over time.

const int Iterations = 8_600;

CertificateAuthority.BuildPrivatePki(
PkiOptions.AllRevocation,
out RevocationResponder responder,
out CertificateAuthority root,
out CertificateAuthority[] intermediates,
out X509Certificate2 endCert,
intermediateAuthorityCount: 4,
registerAuthorities: false,
keyFactory: CertificateAuthority.KeyFactory.RSASize(2048));

using (responder)
using (root)
using (CertificateAuthority intermediate1 = intermediates[0])
using (CertificateAuthority intermediate2 = intermediates[1])
using (CertificateAuthority intermediate3 = intermediates[2])
using (CertificateAuthority intermediate4 = intermediates[3])
using (endCert)
using (ImportedCollection issuerHolder = new ImportedCollection(new X509Certificate2Collection
{
intermediate4.CloneIssuerCert(),
intermediate3.CloneIssuerCert(),
intermediate2.CloneIssuerCert(),
intermediate1.CloneIssuerCert(),
root.CloneIssuerCert(),
}))
using (ChainHolder chainHolder = new ChainHolder())
{
X509Certificate2Collection issuers = issuerHolder.Collection;
X509Chain chain = CreateChain(chainHolder, endCert, issuers);

// Each successful Android chain build materializes the chain from caller-owned JNI
// global references. Without releasing those native-returned references, this
// sequential public-API loop eventually exhausts Android process resources.
for (int i = 0; i < Iterations; i++)
{
if (!chain.Build(endCert))
{
Assert.Fail($"Chain build failed on iteration {i} with '{chain.AllStatusFlags()}'.");
}

if (i == 0)
{
Assert.Equal(issuers.Count + 1, chain.ChainElements.Count);
}

chainHolder.DisposeChainElements();
}
}

static X509Chain CreateChain(ChainHolder chainHolder, X509Certificate2 endCert, X509Certificate2Collection issuers)
{
X509Chain chain = chainHolder.Chain;

chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.VerificationTime = endCert.NotBefore.AddSeconds(1);
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
chain.ChainPolicy.DisableCertificateDownloads = true;
chain.ChainPolicy.ExtraStore.AddRange(issuers);
chain.ChainPolicy.CustomTrustStore.Add(issuers[issuers.Count - 1]);

return chain;
}
}

[Fact]
public static void BuildChainWithSystemTrustAndCustomTrustCertificates()
{
Expand Down
Loading