Skip to content
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

One-shot hash feedback #39372

Merged
merged 4 commits into from
Jul 16, 2020
Merged

One-shot hash feedback #39372

merged 4 commits into from
Jul 16, 2020

Conversation

vcsjones
Copy link
Member

This addresses the feedback from #17590.

I wasn't able to find any good examples of "Use this Win32 API if it's available, otherwise emulate it". If there is a example to model that off of other than what I did, happy to re-do the approach.

Fixes #17590.

@ghost
Copy link

ghost commented Jul 15, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@@ -24,12 +29,86 @@ public static HashProvider CreateMacProvider(string hashAlgorithmId, ReadOnlySpa

public static class OneShotHashProvider
{
private static volatile bool s_UseCompatOneShot;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static volatile bool s_UseCompatOneShot;
private static volatile bool s_useCompatOneShot;

@bartonjs
Copy link
Member

You marked this as fixing the issue, but it isn't doing the EVP algorithm handle caching.

(The easiest fix on that is to just change the internal methods to something like if (s_evpSha1 == IntPtr.Zero) { s_evpSha1 = RealEvpSha1ShimCall(); Debug.Assert(s_evpSha1 != IntPtr.Zero); } return s_evpSha1;)

@vcsjones
Copy link
Member Author

@bartonjs Was this change perhaps overlooked, or am I holding it wrong? https://github.com/dotnet/runtime/pull/39372/files#diff-283eaf9255778de038e266b90f9bea05R32

@bartonjs
Copy link
Member

Ah, looks like the notification I opened was for the second commit being pushed, instead of the root of the PR. I feel better now as to why it wasn't on my screen, but am nonetheless embarrassed and apologetic.

Since I don't see any other calls to those interop methods the place you decided to cache them seems fine.

@vcsjones
Copy link
Member Author

All good! As always I appreciate the quick and thorough reviews.

@bartonjs bartonjs merged commit 6ae5b38 into dotnet:master Jul 16, 2020
@vcsjones vcsjones deleted the 17590-evp-cache branch July 16, 2020 16:40
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add static hash helper methods
5 participants