Spanify and cleanup System.Security.Principal.Windows

[Wraith2]

The code for and around SecurityIdentifier has a number of places where arrays are created only to be passed to a function which will copy their contents. A temporary array of known and small max length is also used when creating one. These can all be made cheaper using stackalloc and spans. This work is in the first commit.

The second commit is a general cleanup and formatting of some code which looks like it's quite old. There are no functional changes just renames and reformat to be within current guidelines and auto code fix applications.

[Wraith2]

I'm not sure what build config is failing. building for frameworks netcoreapp and netfx both work locally but allconfigurations fails and I'm not sure how to track down what flags the cause of the failure are under. It looks like whatever build it is just doesn't have span so I'd add it if I knew.

There's also no specific code owner for this library so as a shot in the dark and knowing that span usage will need a security perspective review I'll @ you @bartonjs .

[stephentoub]

Is this reachable? Wondering if it should be an assert instead.

If it is reachable, is it possible this is now throwing for an empty input whereas previously it would only throw for a null input?

[bartonjs]

This does seem like it would be better as Debug.Assert(subAuthorities.Length > 0). All current callers are passing values downstream of new int[]. The only one that's possibly a concern is CreateFromBinaryForm, if it gets written to hybridize stack and array. But that just means that CreateFromBinaryForm needs to handle the zero case itself. (And probably needs a test that passes 0 sub-authorities, to make sure we don't regress behavior).

[Wraith2]

The check is invalid, 0 is a valid and used length. The null check has been hoist out to the caller for the ctor and no other case can pass in null.

[stephentoub]

Why did you create a local for this? Seems unnecessary, and on top of that it can actually defeat JIT optimizations.

[Wraith2]

I thought I was being helpful. If they JIT wants to do the work of identifying multiple uses of the value I'll let it. Changed.

[stephentoub]

These two lines can just be:

_subAuthorities = subAuthorities.ToArray();

[Wraith2]

Fixed.

[stephentoub]

Does this need to be defined up here, or can it instead be moved down to where it's assigned about 30 lines later?

[Wraith2]

Moved.

[stephentoub]

The unchecked can be removed. Nothing in dotnet/runtime has checked enabled.

[Wraith2]

Done.

[stephentoub]

It is possible this is 0? If yes, then the change made in CreateFromParts is going to throw an exception because the span will be empty, whereas previously it would have accepted a non-null but empty array.

[bartonjs]

Unless something already would have caught it, it's entirely possible... because this is interpreting data from a public ctor (SecurityIdentifier..ctor(byte[], int)).

[Wraith2]

It can be 0 and valid. I've added a max length check and test.

[stephentoub]

Nit: you didn't need to separate this out into a local... you can just change the new int[] to stackalloc int[] in the argument.

[bartonjs]

It feels like a better change is to make internal singletons for all of these comparison SIDs. If they happen to use stackalloc, fine, but it's way less important if they're only ever built once.

[Wraith2]

I've made the statically known ones static lazy initialized singletons. I don't see a point making everyone pay for creating them just because they touch the class i'd prefer that only people who call the methods that need them pay the cost.

[bartonjs]

Personally, I hate this syntax, especially for argument validation. (Mainly because it doesn't compose well, because it leaks half-done work when used for field assignments).

Since there's not a second statement here, and there's unlikely to ever be one, I don't have a technical argument. So I'll just leave it at babbling.

[Wraith2]

It was an autofix suggestion so I have no attachment to it. Informed babbling is reason enough for me, reverted.

[bartonjs]

While the nameof was previously indented wrong, I liked the previous, chopped, style better. This line now causes horizontal scrolling on the default github window width.

[Wraith2]

I've reformatted it. In general i'm not a fan of limiting things because github has a silly limitation though because it feels like limiting line lengths to 72 chars because of terminals.

[bartonjs]

GitHub's limit is 130, and I usually chop around 120 . I think my code window is about 138 characters wide, which generally avoids the need for a horizontal scrollbar.

We don't actually have a repository-wide rule, but in general it's nice to passers-by to avoid h-scroll on GitHub.

[bartonjs]

I don't see anything that complains if binaryForm[offset+1] exceeds MaxSubAuthorities. Please write a test that does so.

I think this is going to give a non-friendly out-of-bounds exception instead of an appropriate "your data is malformed"-type exception.

[bartonjs]

FWIW: Of the changes in this file, I like this one (reduced the line length to not scroll) and dislike the other two (added horizontal scrolling).

[bartonjs]

I'd definitely like to see tests (including just pointing out that they exist) for zero and too-many subidentifiers, to make sure that the exception model didn't change (or to codify that we intentionally change it, if we end up needing to).

[bartonjs]

As for the flavor that's failing to build, I'm guessing either netstandard2.0 or net461:

runtime/src/libraries/System.Security.Principal.Windows/src/Configurations.props

Lines 3 to 10 in adc91a7

	<PackageConfigurations>
	netstandard2.0;
	netcoreapp2.0-Windows_NT;
	netcoreapp2.0-Unix;
	netcoreapp2.1-Windows_NT;
	netcoreapp2.1-Unix;
	net461-Windows_NT;
	</PackageConfigurations>

[bartonjs]

Typically the chop style on this would be what it was. I understand the symmetry here with the bracing rules, so I'm more pointing it out than suggesting a change.

[Wraith2]

I'd like to get rid of

runtime/src/libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

Line 51 in 0812d49

private readonly object _claimsIntiailizedLock = new object();

because it's rarely used. It's used in one place:

runtime/src/libraries/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

Lines 968 to 973 in 0812d49

	private void InitializeClaims()
	{
	if (!_claimsInitialized)
	{
	lock (_claimsIntiailizedLock)
	{

Would it be safe to drop the ctor initialization and add in

if (_claimsIntiailizedLock is null)
{
    Interlocked.CompareExchange(ref _claimsIntiailizedLock, new object(), null);
}

before the lock is taken?

[bartonjs]

If it's really only locked in one place, and also using a boolean as a guard, then perhaps the method can/should be rewritten in terms of LazyInitializer.

private void InitializeClaims()
{
    bool discard = false;

    LazyInitializer.EnsureInitialized(
        ref discard,
        ref _claimsInitialized,
        () =>
        {
            // current lock body
            return true;
        });
}

[Wraith2]

Evrything addressed and updated including the netcoreapp2.0 build issues.

[GrabYourPitchforks]

Overall LGTM - thanks! One notable suggestion re: refactoring involving the subAuthorities local. I realize this suggestion will likely regress a single bounds check optimization, but IMO that's an acceptable tradeoff given the reasoning I listed in the comment.

[GrabYourPitchforks]

Please revert all changes involving the subAuthorities local from this point onward. Essentially, pretend that once the _subAuthorities field is set, the subAuthorities local is set to nullptr. This explicit cut helps lessen the chances of introducing TOCTOU attacks in this code, since we don't want to risk performing operations on the [potentially mutated] subAuthorities local when the _subAuthorities field is instead the source of truth.

[Wraith2]

I've changed the to use the field not the local variable. Would it have been viable to build everything in locals and only assign to fields at the end of the function once everything was known to be ready to move into the constructed state?

[GrabYourPitchforks]

I don't necessarily care about when the field is set. But in particular, the recommendation is that very early on in the method we should make a copy of the data being passed in, and then from that point onward we should only be looking at the copy - not the original source of the data. Whether the copy is immediately assigned to a field or whether the field assignment is deferred to the end of the method isn't really that important IMO.

[Wraith2]

Test failures seem unrelated.

[bartonjs]

System\Security\Principal\SID.cs#L350
System\Security\Principal\SID.cs(350,31): error CS1525: Invalid expression term '='

That one seems like not a flaky test.

[Wraith2]

@David-Engel @cheenamalhotra this will help perf on .net 5 by reducing pool fetch allocations .