if (SecurityIdentifier == null) throws PlatformNotSupportedException on linux

[InterpolationStation]

Description

For better or worse, controlling behavior using nullable objects is a common place pattern. Checking if an object reference is null should never raise an exception. For example,

object? foo = null;
foo?.DoWork();
// or 
if (foo != null)
{
}

Reproduction Steps

Run the following code on Linux.

SecurityIdentifier? securityIdentifier = null;
if (securityIdentifier != null) { }

Expected behavior

(securityIdentifier != null) returns false on Linux and Windows, no exception is raised.

Actual behavior

The following exception is thrown.
System.PlatformNotSupportedException: Windows Principal functionality is not supported on this platform.
at System.Security.Principal.SecurityIdentifier.op_Inequality(SecurityIdentifier left, SecurityIdentifier right)

Regression?

Not sure.

Known Workarounds

No response

Configuration

.net 6.0 linux-x64 self-contained exe

Other information

No response

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

For better or worse, controlling behavior using a nullable object is a common place pattern. Checking if the object reference is null should never raise an exception.

For example,
object? foo = null;
foo?.DoWork(); // execute if the object is not null.
// or
if (foo != null)
{
}

Reproduction Steps

Run the following code on Linux.

SecurityIdentifier? securityIdentifier = null;
if (securityIdentifier != null) { }

Expected behavior

(securityIdentifier != null) returns false on Linux and Windows, no exception is raised.

Actual behavior

The following exception is thrown.
System.PlatformNotSupportedException: Windows Principal functionality is not supported on this platform.
at System.Security.Principal.SecurityIdentifier.op_Inequality(SecurityIdentifier left, SecurityIdentifier right)

Regression?

Not sure.

Known Workarounds

No response

Configuration

.net 6.0 linux-x64 self-contained exe

Other information

No response

Author:	InterpolationStation
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[bartonjs]

This probably isn't something we'd service into .NET 6, since it's been this way since .NET Core 1.0, but I understand your plight, and maybe it's something we can fix for future versions.

Your flow will probably not be altered, but the exception will probably go away, if you change if (securityIdentifier == null) to if (securityIdentifier is null). The problem is that the entirety of the SecurityIdentifier class is auto-generated as throw new PlatformNotSupportedException(the message);, which includes the custom op_Equals method. is null does a nullarity check without calling into the custom op_Equals method, which is why the exception will go away.

[jozkee]

@bartonjs isn't this a broader issue with the auto-generated code that it should instead avoid throwing if one of the arguments is null?

cc @jeffhandley @ViktorHofer

[bartonjs]

If you're asking "should the PNSE generator change implementations of operator== to be something like

public static bool operator==(DeclaringType left, DeclaringType right)
{
    if (left is null && right is null)
    {
        return true;
    }
    
    if (left is null || right is null)
    {
        return false;
    }

    throw new PlatformNotSupportedException(SR.WhateverMessage);
}

public static bool operator !=(DeclaringType left, DeclaringType right) => !(left == right);

?"

Then... perhaps? That is assuming that the type hasn't declared null to be equal to something (which is generally true, and might always be true in the BCL, but isn't guaranteed to be true).

At minimum it might be good to make the PNSE generator fail if it has to implement operator== on a reference type (forcing the not supported type to declare it in a partial), but let it go ahead and define != in terms of ==.

There shouldn't be all that many reference types defining equality operators... it's pretty non-standard in the BCL.

[bartonjs]

Even for a reference type, assuming null always equals null (which.. it... should...), it's probably actually fine to change the generator to the smaller

public static bool operator==(DeclaringType left, DeclaringType right)
{
    if (left is null && right is null)
    {
        return true;
    }

    throw new PlatformNotSupportedException(SR.WhateverMessage);
}

public static bool operator !=(DeclaringType left, DeclaringType right) => !(left == right);

"null is null. Nothing else works". That should match with "this type isn't constructible, because the ctors were also declared to PNSE" (assuming, of course, that the ctors were generated as PNSE and aren't actually implemented, or allowed to use an implicit/default ctor)

[jeffhandley]

I don't know if we've already "broken the seal" and started introducing exceptions to the blanket everything throws PNSE behavior, but I'm reluctant to head in that direction. The is null workaround is nice, and guarding the call with an OperatingSystem check could also be useful here.

[ViktorHofer]

isn't this a broader issue with the auto-generated code that it should instead avoid throwing if one of the arguments is null?

Few months ago we talked about changing the current PNSE algorithm to only throw for static APIs plus object instantiation calls instead of for any API. The current auto-generated source doesn't work well with triple slash XML documentation and roslyn's reference assembly generation feature. We would prefer to represent the PNSE configuration in source. This problem would go away if we would explore that avenue. cc @carlossanlop @ericstj