Open elinor-fung opened 1 month ago
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones See info in area-owners.md if you want to be subscribed.
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones See info in area-owners.md if you want to be subscribed.
@bartonjs possible race with GetRSAPrivateKey
and Dispose
with the new cert loader?
A race in the loader would be exceedingly weird, since anything it disposes would be before returning, and things wouldn't finalize if they were returned out. But taking a quick gander.
The workhorse for this is undoubtedly BuildPrivatePki
(as nothing else instantiates RevocationResponder
). It doesn't use PFX loading, just X.509-DER loading (after CertificateRequest does X.509-DER building) and cert.CopyWithPrivateKey(). So, I don't think loader changes could really be relevant.
System.Net.Security tests do call new X509Certificate2(cert.Export(Pfx))
, but only on the EE cert, which wouldn't be answering revocation prompts.
The places that I see all have all of the objects well-captured in a using... but, this is networking... and that means it's possible that a request got fired off and was being handled on the "server side" concurrent with the test giving up and disposing the certificate and responder it made up for the request.
So... we can remove the assert; but otherwise I think that this is just an inherent state with the nature of asynchronous operations.
We have removed asserts in the past (e.g. https://github.com/dotnet/runtime/pull/86503) where we have checked if a Handle is valid or not. The validity of the handle may not be what we expected in a multi-threaded scenario.
As long as we interact with the handle safely (marshaller or addref + release) we can probably just get rid of the assert and let the handle do its thing by throwing an ObjectDisposedException
.
@bartonjs clarified for me that this is unchanged in .NET 9, so I'm moving it to Future.
From log:
Build Information
Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=759922 Build error leg or test failing: System.Net.Security.Tests.WorkItemExecution Pull request: https://github.com/dotnet/runtime/pull/105673
Error Message
Fill the error message using step by step known issues guidance.
Known issue validation
Build: :mag_right: https://dev.azure.com/dnceng-public/public/_build/results?buildId=759922 Error message validated:
[System.Net.Security.Tests Assertion failed !newKey.IsInvalid
] Result validation: :white_check_mark: Known issue matched with the provided build. Validation performed at: 7/30/2024 5:34:02 PM UTCReport
Summary