Add Span-based overloads to HashAlgorithm and IncrementalHash #23076

stephentoub · 2017-08-09T12:11:50Z

Added IncrementalHash.AppendData/TryGetHashAndReset span-based methods
Added HashAlgorithm.TryComputeHash/HashCore/TryHashFinal span-based methods
Overrode methods on MD5, SHA1, SHA1Managed, SHA256, SHA256Managed, SHA384, SHA384Managed, SHA512, SHA512Managed, HMACMD5, HMACSHA1, HMACSHA256, HMACSHA384, HMACSHA512, MD5CryptoServiceProvider, SHA1CryptoServiceProvider, SHA256CryptoServiceProvider, SHA512CryptoServiceProvider
Added tests
Some minor formatting cleanup along the way to make things more consistent with the new code being added

Fixes https://github.com/dotnet/corefx/issues/22613
Fixes https://github.com/dotnet/corefx/issues/22614
cc: @bartonjs, @KrzysztofCwalina

ghost · 2017-08-09T15:23:39Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/SHA256.cs

-            {
-                return _hashProvider.FinalizeHashAndReset();
-            }
+            protected override void HashCore(ReadOnlySpan<byte> source) =>


Is someone overriding HashCore now? If not, let's be consistent and include the "sealed" keyword. (Same comment applies generally through out this PR.)

The type is sealed. Each method doesn't need to be. I agree we should be consistent, but we should be consistent by removing sealed from all of the methods.

we should be consistent by removing sealed from all of the methods.

I disagree. It's extra work for the reader to scroll up to see whether the class is sealed. Furthermore, if the class is ever unsealed, every method now becomes unsealed when it should be the opposite: methods should only be unsealed when there's a specific need.

We'll need to agree to disagree then. The rest of corefx does not do what you're proposing, and we should be consistent with that.

With regards to the passive-aggressive thumbs down, if you don't like my agree-to-disagree comment, then what exactly do you suggest we do? You don't like the way I've suggested, and I don't like the way you've suggested. My default is to make the entire codebase as consistent as possible, not just this small corner of it, and most of the rest of the codebase relies on a type being sealed to indicate that its methods are also sealed. If someone unseals a type, they and the code reviewers of that change need to understand all of the ramifications of that, not just but including that any unsealed members become overridable (which is of course often why someone chooses to unseal a type, to override its members).

if you don't like my agree-to-disagree comment, then what exactly do you suggest we do?

That depends on what you mean by "agree-to-disagree."

If you mean "I don't want to do that work for this PR because I like it the way it is", that's fine - I don't work much on crypto stuff anymore so I'm not interested in pushing harder on that.

If you mean "You will not use explicit 'sealed' anywhere, and and you will remove them even in sections of the codebase that have maintained this discipline for far longer than corefx has been around", then yes, expect very strong pushback. (That's also not "agree-to-disagree".)

I don't want to do that work for this PR

It has nothing to do with not wanting to do work. It has everything to do with disagreeing with the style.

You will not use explicit 'sealed' anywhere

I have no intention of removing them everywhere; that's a larger discussion that would require more than just my opinion on the matter. Due to the request for consistency, I removed them from the existing methods on the classes to which more methods were being added, so that we're not in a weird state where some methods on these types are explicitly sealed and others aren't.

In the name of making forward progress here, I'll update this with the predominant style of the particular project (which, fwiw, was already inconsistent), and will open an issue to track removing the sealed modifiers from across corefx.

maintained this discipline for far longer than corefx has been around

That doesn't matter. Lots of projects have come together with lots of different styles into corefx, and we're trying to maintain a consistent style across the whole repo.

bartonjs · 2017-08-09T16:23:39Z

...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.OSX.cs

-                Debug.Assert(offset >= 0);
-                Debug.Assert(offset < data.Length);
-                Debug.Assert(count >= 0);
-                Debug.Assert(data.Length - offset > count);


Removing all of these asserts is my favorite thing about Span.

bartonjs · 2017-08-09T16:27:55Z

...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.OSX.cs

+                if (destination.Length < HashSizeInBytes)
+                {
+                    bytesWritten = 0;
+                    return false;


I'd probably do this block before if (!_running). It's not a huge difference, but this block doesn't depend on any side effects from SetKey...

bartonjs · 2017-08-09T16:29:07Z

...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.OSX.cs

                {
-                    byte* pbData = pData + offset;
-                    ret = Interop.AppleCrypto.DigestUpdate(_ctx, pbData, count);
+                    ret = Interop.AppleCrypto.DigestUpdate(_ctx, pData, data.Length);


Maybe this is a dumb question, but: Can't the Marshaller do the DangerousGetPinnableReference for us? Like, should the signature here just be a ReadOnlySpan for the P/Invoke?

Can't the Marshaller do the DangerousGetPinnableReference for us?

Currently, no. It's on the list of things to do to make span more first-class.

If those plans are fairly firm, it might be worth putting a ReadOnlySpan overload in the interop file rather than in the crypto code. That way, the extra DGPR calls are in one folder and are more likely to be removed once the marshaling support is there.

If those plans are fairly firm

I don't know if or when it'll actually happen. But I'm fine moving that into the interop file. I'll do so in a subsequent PR.

bartonjs · 2017-08-09T16:31:00Z

...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.OSX.cs

@@ -235,6 +249,29 @@ public override unsafe byte[] FinalizeHashAndReset()
                return hash;


(This is as close as I can get to the line I care about)

Instead of having this written twice, once for "return array" and once for "write to span", can you make the existing/array one call the new/span one?

Done. We pay for the unnecessary length check, but that's fine.

It looks like this got fixed in the mac version, but the Windows and OpenSsl/Unix versions still have parallel constructions.

I thought I fixed the others. Maybe I neglected to save :( Will look again.

bartonjs · 2017-08-09T16:53:07Z

src/System.Security.Cryptography.Algorithms/tests/HashAlgorithmTest.netcoreapp.cs

+                Assert.True(hash.TryComputeHash(input, actual, out bytesWritten));
+                Assert.Equal(expected.Length, bytesWritten);
+                Assert.Equal(expected, actual);
+                Assert.Throws<NullReferenceException>(() => hash.Hash); // byte[] not initialized


I think we can do better than NullReferenceException. Simply making the property return null when HashValue is null seems legit to me. (MSDN says it's a CryptographicUnexpectedOperationException... though I certainly don't see code backing that up 😄 https://msdn.microsoft.com/en-us/library/system.security.cryptography.hashalgorithm.hash(v=vs.110).aspx)

We should also have a test which codifies what happens to the Hash property when you call ComputeHash(array) followed by TryComputeHash(span). I'm sort of thinking we need to null it out (or whatever) in TryComputeHash.

Simply making the property return null when HashValue is null seems legit to me

Ok... I had that and then undid it, but I've put it back.

We should also have a test which codifies what happens to the Hash property when you call ComputeHash(array) followed by TryComputeHash(span). I'm sort of thinking we need to null it out (or whatever) in TryComputeHash.

Done

bartonjs · 2017-08-09T16:55:04Z

src/System.Security.Cryptography.Algorithms/tests/HashAlgorithmTest.netcoreapp.cs

+
+            using (HashAlgorithm hash = Create())
+            {
+                actual = new byte[expected.Length - 1];


Also a test of array-too-big, value-preserving for the excess values, and bytesWritten being correct?

bartonjs · 2017-08-09T17:00:51Z

src/System.Security.Cryptography.Algorithms/tests/IncrementalHashTests.netcoreapp.cs

+            while (position < s_inputBytes.Length - StepB)
+            {
+                incrementalHash.AppendData(new ReadOnlySpan<byte>(s_inputBytes, position, StepA));
+                position += StepA;


Well, you copied my test as-was... but I'm thinking this while should have used StepB instead of StepA 😄.

Fixed in the original and the copy :)

bartonjs · 2017-08-09T17:05:11Z

src/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/HashAlgorithm.cs

+            if (destination.Length >= HashSizeValue/8)
+            {
+                byte[] final = HashFinal();
+                if (final.Length != HashSizeValue/8)


Save the hashSizeInBytes value to a variable?

Also, it feels like all the other methods were "if too short return false" and this is "if big enough return true". Just seems odd that the style isn't consistent.

Save the hashSizeInBytes value to a variable?

Done

Just seems odd that the style isn't consistent.

Reversed

- Added IncrementalHash.AppendData/TryGetHashAndReset span-based methods - Added HashAlgorithm.TryComputeHash/HashCore/TryHashFinal span-based methods - Overrode methods on MD5, SHA1, SHA1Managed, SHA256, SHA256Managed, SHA384, SHA384Managed, SHA512, SHA512Managed, HMACMD5, HMACSHA1, HMACSHA256, HMACSHA384, HMACSHA512, MD5CryptoServiceProvider, SHA1CryptoServiceProvider, SHA256CryptoServiceProvider, SHA512CryptoServiceProvider - Added tests - Some minor formatting cleanup along the way to make things more consistent with the new code being added

- Rewrite HashProviderDispenser (OSX) FinalizeHashAndReset in terms of TryFinalizeHashAndReset - Reorder _running/SetKey check in HashProviderDispenser - Add bigger-than-needed-span test for HashAlgorithm - Fix incorrect const usage in original and copied test - Change HashAlgorithm.Hash to avoid throwing if HashValue is null - Make TryComputeHash null out HashValue on success - Cache HashSizeValue/8 into a local in TryHashFinal rather than recomputing

steji113 · 2017-08-10T22:19:24Z

@stephentoub I am wondering if you could help clarify a few bullet points in the CoreFX contributing guide here. For example, I have noticed many PRs tend to mix code style / cleanup changes with actual functional changes, i.e. using expression bodied members, null-conditionals, etc. Particularly at issue seem to be the following bullets:

DO NOT send PRs for style changes. For example, do not send PRs that are focused on changing usage of Int32 to int.

DO NOT send PRs for upgrading code to use newer language features, though it's ok to use newer language features as part of new code that's written. For example, it's ok to use expression-bodied members as part of new code you write, but do not send a PR focused on changing existing properties or methods to use the feature.

DO NOT mix independent, unrelated changes in one PR. Separate real product/test code changes from larger code formatting/dead code removal changes. Separate unrelated fixes into separate PRs, especially if they are in different assemblies.

Is the contributing guide still accurate in these cases, such that contributors should be strictly following these? Also, in a related question, are there still plans to bring the CoreFX code base up to the latest style guidelines in addition to leveraging newer language features as referenced here?:

We intend to bring dotnet/corefx into full conformance with the style guidelines described in Coding Style. We plan to do that with tooling, in a holistic way.

I just want to make sure I am following the appropriate guidelines when making contributions and also am willing to help out with targeted style / language feature improvements. Thanks!

stephentoub · 2017-08-11T04:03:42Z

Is the contributing guide still accurate in these cases

We've gotten much more lax in this regard since the guidance was originally written. We'd still prefer not to get a PR focused on such style changes, but we're more accepting of such changes being made in the same PR as other changes for code near or impacted by those changes.

steji113 · 2017-08-11T13:44:22Z

OK great thanks for the clarification!

…/corefx#23076) * Add Span-based overloads to HashAlgorithm and IncrementalHash - Added IncrementalHash.AppendData/TryGetHashAndReset span-based methods - Added HashAlgorithm.TryComputeHash/HashCore/TryHashFinal span-based methods - Overrode methods on MD5, SHA1, SHA1Managed, SHA256, SHA256Managed, SHA384, SHA384Managed, SHA512, SHA512Managed, HMACMD5, HMACSHA1, HMACSHA256, HMACSHA384, HMACSHA512, MD5CryptoServiceProvider, SHA1CryptoServiceProvider, SHA256CryptoServiceProvider, SHA512CryptoServiceProvider - Added tests - Some minor formatting cleanup along the way to make things more consistent with the new code being added * Address PR feedback - Rewrite HashProviderDispenser (OSX) FinalizeHashAndReset in terms of TryFinalizeHashAndReset - Reorder _running/SetKey check in HashProviderDispenser - Add bigger-than-needed-span test for HashAlgorithm - Fix incorrect const usage in original and copied test - Change HashAlgorithm.Hash to avoid throwing if HashValue is null - Make TryComputeHash null out HashValue on success - Cache HashSizeValue/8 into a local in TryHashFinal rather than recomputing Commit migrated from dotnet/corefx@154c2a3

dnfclas added the cla-already-signed label Aug 9, 2017

stephentoub force-pushed the hash_spans branch from a64c8e4 to 22c6e4e Compare August 9, 2017 12:40

ghost reviewed Aug 9, 2017

View reviewed changes

bartonjs reviewed Aug 9, 2017

View reviewed changes

stephentoub force-pushed the hash_spans branch 2 times, most recently from 4235371 to a786b2a Compare August 9, 2017 19:58

stephentoub force-pushed the hash_spans branch from a786b2a to 0597b42 Compare August 10, 2017 03:43

stephentoub force-pushed the hash_spans branch from 0597b42 to 77656d4 Compare August 10, 2017 11:35

stephentoub merged commit 154c2a3 into dotnet:master Aug 10, 2017

stephentoub deleted the hash_spans branch August 10, 2017 13:05

karelz modified the milestone: 2.1.0 Aug 14, 2017

stephentoub added the netfx-port-consider label Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Span-based overloads to HashAlgorithm and IncrementalHash #23076

Add Span-based overloads to HashAlgorithm and IncrementalHash #23076

stephentoub commented Aug 9, 2017

ghost Aug 9, 2017

stephentoub Aug 9, 2017

ghost Aug 9, 2017

stephentoub Aug 9, 2017

stephentoub Aug 9, 2017 •

edited

Loading

ghost Aug 9, 2017 •

edited by ghost

Loading

stephentoub Aug 9, 2017

stephentoub Aug 9, 2017 •

edited

Loading

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

ghost Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

bartonjs Aug 9, 2017

stephentoub Aug 9, 2017

steji113 commented Aug 10, 2017

stephentoub commented Aug 11, 2017

steji113 commented Aug 11, 2017

		@@ -235,6 +249,29 @@ public override unsafe byte[] FinalizeHashAndReset()
		return hash;

Add Span-based overloads to HashAlgorithm and IncrementalHash #23076

Add Span-based overloads to HashAlgorithm and IncrementalHash #23076

Conversation

stephentoub commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

ghost Aug 9, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steji113 commented Aug 10, 2017

stephentoub commented Aug 11, 2017

steji113 commented Aug 11, 2017

stephentoub Aug 9, 2017 •

edited

Loading

ghost Aug 9, 2017 •

edited by ghost

Loading

stephentoub Aug 9, 2017 •

edited

Loading