Avoid finalizable internal state for non-FIPS scenarios #67998
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
yeah, my pref would be that thsi only be a fallback on netfx. :) could we also get stephen to look? |
| @@ -206,7 +206,7 @@ internal static ImmutableArray<byte> ComputeHash(HashAlgorithmName algorithmName | |||
| internal static ImmutableArray<byte> ComputeSourceHash(ImmutableArray<byte> bytes, SourceHashAlgorithm hashAlgorithm = SourceHashAlgorithms.Default) | |||
| { | |||
| var algorithmName = GetAlgorithmName(hashAlgorithm); | |||
| using (var incrementalHash = IncrementalHash.CreateHash(algorithmName)) | |||
| using (var incrementalHash = RoslynIncrementalHash.CreateHash(algorithmName)) | |||
| { | |||
| incrementalHash.AppendData(bytes.ToArray()); | |||
There was a problem hiding this comment.
yikes. feels like this should be better on Core as well, by using available hash functions that don't require tehse intermediary allocations.
|
Could you link to the difference in this and teh existing netfx impl that addresses the core problem? i want to understand the difference better. (would also be good if the code itself potentially had such a link. thanks!) |
| if (hashAlgorithm == HashAlgorithmName.MD5) | ||
| { | ||
| return MD5.Create(); | ||
| } | ||
|
|
||
| if (hashAlgorithm == HashAlgorithmName.SHA1) | ||
| { | ||
| return SHA1.Create(); | ||
| } | ||
|
|
||
| if (hashAlgorithm == HashAlgorithmName.SHA256) | ||
| { | ||
| return SHA256.Create(); | ||
| } | ||
|
|
||
| if (hashAlgorithm == HashAlgorithmName.SHA384) | ||
| { | ||
| return SHA384.Create(); | ||
| } | ||
|
|
||
| if (hashAlgorithm == HashAlgorithmName.SHA512) | ||
| { | ||
| return SHA512.Create(); | ||
| } |
There was a problem hiding this comment.
📝 The change relative to the reference implementation is here. .NET Framework creates instances of SHA256CryptoServiceProvider instead of calling SHA256.Create() (likewise for each of the others).
There was a problem hiding this comment.
💭 In theory, we could also address this by modifying the way we create our pool of IncrementalHash objects to use reflection to reset the _hash field.
There was a problem hiding this comment.
and it's hte provider that was causing the problem before?
|
Closing this in favor of #67999 |
Fixes #67995
Marked as draft since the current approach focuses exclusively on .NET Framework execution. Need to determine if this will result in a penalty for cases that were previously using
IncrementalHashon .NET Core.