Skip to content

Add Dispose for X509Chain instance #110740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
krwq merged 7 commits into from
Jan 20, 2025
Merged

Add Dispose for X509Chain instance #110740

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09af542
Add Dispose for X509Chain instance
MaxPatri Dec 16, 2024
1655745
Review fix. Add try/finally
MaxPatri Jan 14, 2025
785577f
Fix chain initialization
MaxPatri Jan 15, 2025
9fd6c3c
Add empty line
MaxPatri Jan 15, 2025
3ea8658
Merge branch 'main' into SignerInfo-dispose-chain
MaxPatri Jan 15, 2025
469b268
Merge branch 'main' into SignerInfo-dispose-chain
MaxPatri Jan 15, 2025
aa984d4
Merge branch 'main' into SignerInfo-dispose-chain
MaxPatri Jan 17, 2025
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
2 changes: 1 addition & 1 deletion ...ies/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ private void Verify(

if (!verifySignatureOnly)
{
X509Chain chain = new X509Chain();
using X509Chain chain = new X509Chain();
Copy link
Member

@bartonjs bartonjs Jan 10, 2025

Choose a reason for hiding this comment

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

Instead of the implicit scope using, this should be a normal block-body using statement, and the end of the block should be at the end of the if that calls chain.Build() (so between that block and the EKU check)

chain.ChainPolicy.ExtraStore.AddRange(extraStore);
chain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot;
Copy link
Member

@bartonjs bartonjs Jan 10, 2025

Choose a reason for hiding this comment

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

If we're trying to reduce finalizations, then line 714 probably wants to change to

bool trustedChain = chain.Build(certificate);

for (int i = 0; i < chain.ChainElements.Length; i++)
{
    chain.ChainElements[i].Certificate.Dispose();
}

if (!trustedChain)

Or instead of a using here make it a try/finally and put that cleanup in the finally.

The certificate instances in ChainElements are all new instances (copies) of the input objects, which can be seen via the NotSame assertions in

runtime/src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs

Line 557 in 9a5f012

Assert.NotSame(cert, chain.ChainElements[0].Certificate);
(and the rest of the file); so there's no impact from extraStore or what have you.

Copy link
Contributor Author

@MaxPatri MaxPatri Jan 14, 2025

Choose a reason for hiding this comment

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

Added try/finally

Expand Down