Fix Windows 8 detection.

[teo-tsirpanis]

Windows 8's actual version is 6.2, not 8. The test would succeed only on Windows 10+.

[teo-tsirpanis]

I also noticed in the same file some spans being pinned without MemoryMarshal.GetReference. Was that intentional or can I change it?

[KalleOlaviNiemitalo]

Is there a policy that requires MemoryMarshal.GetReference? I did not find one in https://github.com/dotnet/runtime/tree/main/docs/coding-guidelines. ReadOnlySpan<T>.GetPinnableReference() exists in the same target frameworks.

[teo-tsirpanis]

@KalleOlaviNiemitalo I have seen it being used almost always when pinning spans within the BCL.

[KalleOlaviNiemitalo]

A fixed statement using MemoryMarshal.GetReference was most recently added in f4e88f4 on 2021-07-02, I think. But 6e286dd on 2021-08-31 used fixed on a Span directly. Perhaps the MemoryMarshal.GetReference style has been left over from runtime versions that were compiled as C# 7.2 or lower.

[stephentoub]

Is there a policy that requires MemoryMarshal.GetReference?

There's no policy one way or the other. Pinning with:

fixed (byte* ptr = &MemoryMarshal.GetReference(span))

is ever so slightly more efficient than with:

fixed (byte* ptr = span)

as the latter ends up using span.GetPinnableReference, which is identical to MemoryMarshal.GetReference except that the former involves an extra length check; GetPinnableReference will return a null ref (e.g. Unsafe.NullRef<T>) if the span has a Length of 0, whereas MemoryMarshal.GetReference will return whatever reference the span contains, even if the span is empty. Some APIs don't like it when you give them a bad pointer even if they won't end up dereferencing it due to a length/count being zero.

[vcsjones]

This looks good to me. Thanks for the fix.

[vcsjones]

How feasible would it be to get this in 6.0, @stephentoub / @dotnet/area-system-security? The impact on this is basically Windows 8.0 will use a much slower Windows 7 API for the PBKDF2 one-shot.

[jeffhandley]

While this doesn't directly align with any of our RC2 bar criteria, I support presenting it to tactics. After it's merged into RC2, let's backport it to release/6.0 and label it for consideration.

[GrabYourPitchforks]

I also support bringing this to tactics. Low-risk, good test coverage, etc.

[jeffhandley]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1214551203

[jeffhandley]

Thanks for finding and fixing this, @teo-tsirpanis!