Add Dispose for X509Chain instance #110740
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
The using looks good but since we're touching this code I noticed we might also not dispose of ChainElements correctly but given we're also touching extraStore I need to consult this with some folks how to correctly do it before we proceed (I will try to find the answer this week but I'm out of office and given the holidays period this might take until sometime January)
|
OK! Thanks! |
|
cc: @bartonjs |
| @@ -706,7 +706,7 @@ private void Verify( | |||
|
|
|||
| if (!verifySignatureOnly) | |||
| { | |||
| X509Chain chain = new X509Chain(); | |||
| using X509Chain chain = new X509Chain(); | |||
There was a problem hiding this comment.
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)
| @@ -706,7 +706,7 @@ private void Verify( | |||
|
|
|||
| if (!verifySignatureOnly) | |||
| { | |||
| X509Chain chain = new X509Chain(); | |||
| using X509Chain chain = new X509Chain(); | |||
| chain.ChainPolicy.ExtraStore.AddRange(extraStore); | |||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.Online; | |||
| chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; | |||
There was a problem hiding this comment.
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
extraStore or what have you.
| chain.ChainPolicy.ExtraStore.AddRange(extraStore); | ||
| chain.ChainPolicy.RevocationMode = X509RevocationMode.Online; | ||
| chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; | ||
| X509Chain chain = null; |
There was a problem hiding this comment.
The X509Chain ctor doesn't do any work, so it can't throw. You can either declare this as nullable or just merge the new X509Chain() onto this line.
...raries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfo.cs
Show resolved
Hide resolved
|
@MaxPatri can you please refrain from adding merge commits to this branch? |
|
@krwq Yes, I was restarting failed build. If the another way to do this exists, please, let me know. |
|
I can just rerun github checks (not sure if you have permissions for that as well) - we can mostly see if failures are related and there is also Build Analysis check which does that semi-automatically but pushing new changes requires us to check if nothing changed in the PR on top of that and unnecessarily wastes many CPU cycles |
|
Merging since the errors are not related to this PR and are wasm specific |
* main: (89 commits) Add Dispose for X509Chain instance (dotnet#110740) Fix XML comment on regex split enumerator (dotnet#111572) JIT: tolerate missing InitClass map in SPMI (dotnet#111555) Build ilasm/ildasm packages for the host machine (dotnet#111512) Unicode 16.0 Support (dotnet#111469) Improve performance of interface method resolution in ILC (dotnet#103066) Fix building the host-targeting components and packing ILC (dotnet#111552) Improve JSON validation perf (dotnet#111332) Update github-merge-flow.jsonc to autoflow 9.0 to 9.0-staging (dotnet#111549) Include GPL-3 licence text in the notice (dotnet#111528) Remove explicit __compact_unwind entries from x64 assembler (dotnet#111530) Add MemoryExtensions overloads with comparer (dotnet#110197) Avoid capturing the ExecutionContext for the whole HTTP connection lifetime (dotnet#111475) Forward DefaultArtifactVisibility down from the VMR orchestrator (dotnet#111513) Fix relocs errors on riscv64 (dotnet#111317) Added JITDUMP_USE_ARCH_TIMESTAMP support. (dotnet#111359) add rcl/rcr tp and latency info (dotnet#111442) Fix stack overflow in compiler-generated state (dotnet#109207) Produce a package with the host-running ILC for repos in the VMR (dotnet#111443) Delete dead code in ilasm PE writer (dotnet#111218) ...
Call Dispose for X509Chain class instance
Found by Linux Verification Center (linuxtesting.org) with SVACE.