Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed#100513

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.

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

Already on GitHub?Sign in to your account

Conversation

@rzikm
Copy link
Member

@rzikmrzikm commentedApr 2, 2024
edited
Loading

Closes#95687.

The affected code path is on Windows in mutually authenticated connections (i.e. with client certificate), and where client accesses either of:

  • IsMutuallyAuthenticated
  • LocalCertificate

Server side is unaffected, as well as client side without client certificate.

BenchmarkDotNet v0.13.11-nightly.20231126.107, Windows 11 (10.0.22631.3296/23H2/2023Update/SunValley3)Intel Core i9-10900K CPU 3.70GHz, 1 CPU, 20 logical and 10 physical cores.NET SDK 8.0.300-preview.24201.7  [Host]     : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX2  Job-JOWNWG : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2  Job-UGFCLX : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250.0000 ms  MaxIterationCount=20  MinIterationCount=15  WarmupCount=1
MethodJobToolchainMeanErrorStdDevMedianMinMaxRatioAllocatedAlloc Ratio
IsMutuallyAuthenticatedJob-JOWNWG\95687-SslStreamIsMutuallyAuthenticated-calls-are-expensive-on-Windows\corerun.exe5.271 ns0.0428 ns0.0334 ns5.273 ns5.202 ns5.320 ns0.000-0.00
IsMutuallyAuthenticatedJob-UGFCLX\main\corerun.exe62,898.672 ns1,125.7018 ns997.9053 ns63,306.704 ns61,270.539 ns64,574.663 ns1.00032 B1.00

token.ReleasePayload();

// reset the cached flag which has potentially outdated value.
_localClientCertificateUsed=null;
Copy link
Member

Choose a reason for hiding this comment

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

Are there threading issues here? bool? is two bool fields (one for null or not, one for the value) and they're not necessarily updated or read atomically. If someone was reading IsLocalClientCertificateUsed while this was being reset, might they end up reading an incorrect value, e.g. if this flipped the value from true to false before flipping the null state?

Copy link
MemberAuthor

@rzikmrzikmApr 4, 2024
edited
Loading

Choose a reason for hiding this comment

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

The value can go only from false to true, and that happens only during renegotiation/post-handshake authentication, which is usually at most once per connection.

The only thing I can think of is

  • thread 1 writes HasValue = false (part of the null assignment)
  • thread 2 accesses IsLocalClientCertificateUsed, assigning HasValue = true, Value = true
  • thread 1 writes Value = false (if that is how null assignment work)

Actual calculation of the new value is expensive (which is why I wanted to cache it) so this ordering is not likely, but still possible in theory.

If you still see any concerns than I think we can either move to something like Lazy (at the cost of small allocation) or fetching the new value here (potential small perf hit on (windows) client in mutual auth scenario).

Copy link
Member

Choose a reason for hiding this comment

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

I'd just change the storage to something written atomically, e.g. use an int that's 0 for uninitialized, 1 for true, -1 for false.

rzikm reacted with thumbs up emoji
@rzikmrzikm requested a review fromstephentoubApril 4, 2024 18:18
@rzikmrzikm merged commitbc2bd2b intodotnet:mainApr 8, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull requestApr 30, 2024
…ed (dotnet#100513)* Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed* Code review feedback
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 8, 2024
@karelzkarelz added this to the9.0.0 milestoneMay 14, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes

@stephentoubstephentoubAwaiting requested review from stephentoub

Assignees

@rzikmrzikm

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

SslStream.IsMutuallyAuthenticated calls are expensive on Windows

4 participants

@rzikm@stephentoub@wfurt@karelz

[8]ページ先頭

©2009-2025 Movatter.jp