Fix missing .NET Core argument validation for PrivateFontCollection.AddFontFile

[hughbe]

This is a cleaned up copy of the netfx source code.

However, is the whole IntSecurity.DemandReadFileIO(filename) stuff necessary

Or maybe I could just replace that with filename = Path.GetFullPath(filename) and delete IntSecurity.cs

Fixes #21558

[stephentoub]

is the whole IntSecurity.DemandReadFileIO(filename) stuff necessary

Nope :)

[danmoseley]

@hughbe I believe all use of System.Security.Permissions.* (and using statements) can be removed from Drawing. (Almost) all of our implmentations of those types are stubs.

[hughbe]

I asked before but it may have gone unnoticed. Does that apply even for Mono?

I've updated this PR after Stephen commented to get rid of the security stuff

[mellinoe]

This is a cleaned up copy of the netfx source code.

Maybe I'm misinterpreting this, but I don't think the .NET Framework version does this normalization. Could you clear that up? It looks like an okay change, but TBH I'd rather just keep 100% behavior compat with .NET Framework until we have excellent test coverage.

[hughbe]

Before this cleanup, it used IntSecurity.DemandReadFileIO. This was unnecessary in netcoreapp as Stephen mentioned so I could just shorten it to Path.GetFullPath

[mellinoe]

I get that it used IntSecurity.DemandReadFileIO(filename); before, but that shouldn't affect which value is passed into GdipPrivateAddFontFile, right? From my understanding DemandReadFileIO will just do a security / permissions check on that full path, and that's the part we no longer care about.

[hughbe]

The code in netfx is:

public void AddFontFile (string filename) {
    IntSecurity.DemandReadFileIO(filename);
    int status = SafeNativeMethods.Gdip.GdipPrivateAddFontFile(new HandleRef(this, nativeFontCollection), filename);

    if (status != SafeNativeMethods.Gdip.Ok)
        throw SafeNativeMethods.Gdip.StatusException(status);
        
    // Register private font with GDI as well so pure GDI-based controls (TextBox, Button for instance) can access it.
    SafeNativeMethods.AddFontFile( filename );
}

We got rid of the IntSecurity.DemandReadFileIO(filename); part presumably because .NET Core doesn't need the security/permissions bit.

However, this method has side effects that weren't evident at the time of porting due to no tests.

Namely, that it called UnsafeGetFullPath before demanding the permissions

internal static void DemandReadFileIO(string fileName) {
  string full = fileName;
  
  full = UnsafeGetFullPath(fileName);

  new FileIOPermission(FileIOPermissionAccess.Read, full).Demand();
}

[ResourceExposure(ResourceScope.Machine)]
[ResourceConsumption(ResourceScope.Machine)]
internal static string UnsafeGetFullPath(string fileName) {
    string full = fileName;

    FileIOPermission fiop = new FileIOPermission(PermissionState.None);
    fiop.AllFiles = FileIOPermissionAccess.PathDiscovery;
    fiop.Assert();

    try {
        full = Path.GetFullPath(fileName);
    } finally {
        CodeAccessPermission.RevertAssert();
    }

    return full;
}

So we'd get to the call full = Path.GetFullPath(fileName);, which can and does throw in the situation this PR fixes.

but that shouldn't affect which value is passed into GdipPrivateAddFontFile, right?

The value passed to GdipPrivateAddFontFile is not affected by DemadReadFileIO, correct

[mellinoe]

The value passed to GdipPrivateAddFontFile is not affected by DemadReadFileIO, correct

I'm trying to point out that this change does affect the call to GdipPrivateAddFontFile:

filename = Path.GetFullPath(filename);
int status = SafeNativeMethods.Gdip.GdipPrivateAddFontFile(new HandleRef(this, _nativeFontCollection), filename);

[hughbe]

Ohhhhh dammit! Sorry I wasn't really getting that. It shouldn't actually matter as GdipPrivateAddFontFile handles relative file paths anyways (and we have a test for that already)

But I removed the assignment :)

[mellinoe]

LGTM. Thanks, @hughbe