-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed #100513
Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed #100513
Conversation
…-are-expensive-on-Windows
@@ -360,6 +360,9 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[ | |||
} | |||
|
|||
token.ReleasePayload(); | |||
|
|||
// reset the cached flag which has potentially outdated value. | |||
_localClientCertificateUsed = null; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
…ed (dotnet#100513) * Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed * Code review feedback
Closes #95687.
The affected code path is on Windows in mutually authenticated connections (i.e. with client certificate), and where client accesses either of:
Server side is unaffected, as well as client side without client certificate.