Gentle ping @halter73

cc @janvorli

cc: @rzikm

You may also want to dispose certificate at this callsite

https://github.com/jashook/aspnetcore/blob/cf7235c3a0b8fc5a841dd4838fdea4975b5d3557/src/Shared/CertificateGeneration/UnixCertificateManager.cs#L111

Note that this PR doesn't fix a "leak" per se. The cert instances will be eventually collected by GC and finalization will ensure the native resources are released

Yes, and no, for us the rate at which we do tls handshakes outpaces the rate of gc. Which leads to unbounded memory growth, until the gc collects aggressively, at which the application will health check and die.

As in, yes you are correct this is not a native leak from the runtime, but it is effectively a managed leak with native resources which leads to the application degrading and restarting.

cc @leculver

Will address the comment. Can we take this into net8?

You may also want to dispose certificate at this callsite

https://github.com/jashook/aspnetcore/blob/cf7235c3a0b8fc5a841dd4838fdea4975b5d3557/src/Shared/CertificateGeneration/UnixCertificateManager.cs#L111

😅 lol nice catch...

Seems like this can be merged. cc @halter73

Gentle ping

Thanks for the contribution, @jashook!

Can we take this into net8? @MackinnonBuck

Gentle ping @MackinnonBuck

Thanks, @jashook. We'll consider this fix for servicing in .NET 8 and .NET 9.

cc @bartonjs in case you have thoughts about the severity of this issue.

The general severity is pretty low. As was already pointed out, the GC will eventually catch up and release all of the stuff. But, since you have an external person asking for it, that gives it a boost.

The risk is also pretty low. And the change looks sound.

I don't actually participate in the servicing reviews, but, if I did, since it has a requestor, is straightforward, and isn't blazing any trails... I'd vote yes.

As was already pointed out, the GC will eventually catch up and release all of the stuff.

Right, I responded to this. It is an incorrect way of viewing this problem. It leaves this up to a game of chance between rate of connection establishment and gen0 gc count. In our case, we have high rate of re-establishments which are not going away quickly, and we want to keep our gen 0-1 GC count as low as possible. There is a workaround; however, I guarantee that Roblox is not the only effected customer here.

I don't actually participate in the servicing reviews, but, if I did, since it has a requestor, is straightforward, and isn't blazing any trails... I'd vote yes.

Could we tag who is needed to get this into the next shiproom discussion? Thank you!

Note for anyone else stumbling on this, we are working around this issue by using SslStreamCertificateContext to build the chain once and cache it.

/backport to release/9.0

Started backporting to release/9.0: https://github.com/dotnet/aspnetcore/actions/runs/16605934603

/backport to release/8.0

Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/16605950943

@MackinnonBuck backporting to "release/8.0" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Dispose the certificate chain elements with the chain
Applying: Fix the missing brace
Applying: Remove snarky comment.
Applying: Add another choice using based on review feedback
error: sha1 information is lacking or useless (src/Shared/CertificateGeneration/UnixCertificateManager.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0004 Add another choice using based on review feedback
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Note for anyone else stumbling on this, we are working around this issue by using SslStreamCertificateContext to build the chain once and cache it.

@jashook would you mind elaborating a bit how that is expected to help? the changed code paths are on remote cert validation paths which don't create their own SslStreamCerificateContext.

If you mean that that you use cached SslStreamCertificateContext to specify local certificates, that is the recommended practice, especially if your app sees higher amount of traffic (in addition to avoiding rebuilding the cert chain, it also enables TLS Resume on Linux). AFAIK In case of server certificate, unless you use the certificate selection callback, the ASP.NET Core runtime will even cache the context for you automatically. Making sure that SslStreamCertificateContext is not created fresh for each connection is just another way to save CPU cycles and reduce the number of X509Certificate2 instances flying around and is orthogonal to changes in this PR.