Cannot set ReadMode after creating a NamedPipeClientStream instance

[carlossanlop]

Background and motivation

Cannot set ReadMode after creating a NamedPipeClientStream instance. This is due to not exposing the NamedPipeClientStream constructor which accepts a PipeAccessRights.

API Proposal

namespace System.IO.Pipes;

// existing type
public sealed partial class NamedPipeClientStream : PipeStream
{
    // new constructor
    [SupportedOSPlatform("windows")]
    public NamedPipeClientStream(String serverName, String pipeName, PipeAccessRights desiredAccessRights,
           PipeOptions options, TokenImpersonationLevel impersonationLevel, HandleInheritability inheritability)
}

// existing type moved from System.IO.Pipes.AccessControl assembly.
[System.FlagsAttribute]
public enum PipeAccessRights
{
    ReadData = 1,
    WriteData = 2,
    CreateNewInstance = 4,
    ReadExtendedAttributes = 8,
    WriteExtendedAttributes = 16,
    ReadAttributes = 128,
    WriteAttributes = 256,
    Write = 274,
    Delete = 65536,
    ReadPermissions = 131072,
    Read = 131209,
    ReadWrite = 131483,
    ChangePermissions = 262144,
    TakeOwnership = 524288,
    Synchronize = 1048576,
    FullControl = 2032031,
    AccessSystemSecurity = 16777216,
}

API Usage

var pipeOut = new NamedPipeServerStream("SomeNamedPipe",
                                        PipeDirection.Out,
                                        1,
                                        PipeTransmissionMode.Message);

var pipeIn = new NamedPipeClientStream(".", 
                             "SomeNamedPipe", 
                             PipeAccessRights.ReadData | PipeAccessRights.WriteAttributes, 
                             PipeOptions.None, 
                             System.Security.Principal.TokenImpersonationLevel.None, 
                             System.IO.HandleInheritability.None);

pipeIn.Connect();
pipeIn.ReadMode = PipeTransmissionMode.Message; 

Alternative Designs

We could expose this with a Create factory method instead, as was done for NamedPipeServerStream

runtime/src/libraries/System.IO.Pipes.AccessControl/ref/System.IO.Pipes.AccessControl.cs

Lines 9 to 16 in dbb335c

	public static class AnonymousPipeServerStreamAcl
	{
	public static System.IO.Pipes.AnonymousPipeServerStream Create(System.IO.Pipes.PipeDirection direction, System.IO.HandleInheritability inheritability, int bufferSize, System.IO.Pipes.PipeSecurity? pipeSecurity) { throw null; }
	}
	public static class NamedPipeServerStreamAcl
	{
	public static System.IO.Pipes.NamedPipeServerStream Create(string pipeName, System.IO.Pipes.PipeDirection direction, int maxNumberOfServerInstances, System.IO.Pipes.PipeTransmissionMode transmissionMode, System.IO.Pipes.PipeOptions options, int inBufferSize, int outBufferSize, System.IO.Pipes.PipeSecurity? pipeSecurity, System.IO.HandleInheritability inheritability = System.IO.HandleInheritability.None, System.IO.Pipes.PipeAccessRights additionalAccessRights = default) { throw null; }
	}

However that's not necessary and probably a misnomer since this new constructor isn't really about ACLs, but instead access rights on the handle. Contrast that to those existing factory methods which take PipeSecurity which is an ACLs concept. Splitting hairs, perhaps, since the API is related to access and is likely not supported on non-Windows, but I think we can do better here by exposing the exact missing API so we should.

Risks

Moving types around carries with it some risk. Luckily all the assemblies here are in the shared framework (since 6.0) and referenced as a set so it's less of a risk,

Original issue

Consider this code:

var pipeOut = new NamedPipeServerStream("SomeNamedPipe",
                                        PipeDirection.Out,
                                        1,
                                        PipeTransmissionMode.Message);

var pipeIn = new NamedPipeClientStream(".",
                                       "SomeNamedPipe",
                                       PipeDirection.In);

pipeIn.Connect();
pipeIn.ReadMode = PipeTransmissionMode.Message;  // <-- exception thrown here

In .NET Framework, there used to be a constructor on the client side that accepted PipeAccessRights, which would solve the issue, and would allow the user to change ReadMode later:

var pipeIn = 
   new NamedPipeClientStream(".", 
                             "SomeNamedPipe", 
                             PipeAccessRights.ReadData | PipeAccessRights.WriteAttributes, 
                             PipeOptions.None, 
                             System.Security.Principal.TokenImpersonationLevel.None, 
                             System.IO.HandleInheritability.None);

Unfortunately, we haven't ported that constructor to .NET Core+.

The underlying problem is that, according to the documentation of the Windows method CreateNamedPipe, we need to pass the FILE_WRITE_ATTRIBUTES access right when constructing the pipe handle, to be able to modify it after creation. This is explained in the remarks: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea#remarks

The .NET Framework solution worked because it would set FILE_WRITE_ATTRIBUTES when using the constructor that takes a PipeAccessRights argument.

Unfortunately, the only workaround available in .NET Core+ is to:

• Manually construct the SafePipeHandle, making sure to pass FILE_WRITE_ATTRIBUTES to the P/Invoke, which is not straightforward.
• Create the client stream instance using the constructor that takes the pipe handle, while also making sure to pass true to the argument that indicates that the pipe should get connected.
• Set the ReadMode.

I wrote a working example of this workaround for .NET 6.0 and published it here: https://github.com/carlossanlop/experiments/tree/NamedPipeClientStream_ReadMode

But we should consider solving it in runtime.

Note that the workaround provided above had to call an internal constructor from PipeSecurity using reflection. It's another argumemt supporting the idea that we should provide the solution ourselves.

[ghost]

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

Issue Details

---

Consider this code:

var pipeOut = new NamedPipeServerStream("SomeNamedPipe",
                                        PipeDirection.Out,
                                        1,
                                        PipeTransmissionMode.Message);

var pipeIn = new NamedPipeClientStream(".",
                                       "SomeNamedPipe",
                                       PipeDirection.In);

pipeIn.Connect();
pipeIn.ReadMode = PipeTransmissionMode.Message;  // <-- exception thrown here

In .NET Framework, there used to be a constructor on the client side that accepted PipeAccessRights, which would solve the issue, and would allow the user to change ReadMode later:

var pipeIn = 
   new NamedPipeClientStream(".", 
                             "SomeNamedPipe", 
                             PipeAccessRights.ReadData | PipeAccessRights.WriteAttributes, 
                             PipeOptions.None, 
                             System.Security.Principal.TokenImpersonationLevel.None, 
                             System.IO.HandleInheritability.None);

Unfortunately, we haven't ported that constructor to .NET Core+.

The underlying problem is that, according to the documentation of the Windows method CreateNamedPipe, we need to pass the FILE_WRITE_ATTRIBUTES access right when constructing the pipe handle, to be able to modify it after creation. This is explained in the remarks: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea#remarks

The .NET Framework solution worked because it would set FILE_WRITE_ATTRIBUTES when using the constructor that takes a PipeAccessRights argument.

Unfortunately, the only workaround available in .NET Core+ is to:

• Manually construct the SafePipeHandle, making sure to pass FILE_WRITE_ATTRIBUTES to the P/Invoke, which is not straightforward.
• Create the client stream instance using the constructor that takes the pipe handle, while also making sure to pass true to the argument that indicates that the pipe should get connected.
• Set the ReadMode.

I wrote a working example of this workaround for .NET 6.0 and published it here: https://github.com/carlossanlop/experiments/tree/NamedPipeClientStream_ReadMode

But we should consider solving it in runtime.

Note that in .NET Core+ we use FileCreate to create the handle (invoked from here), while in .NET Framework we use CreateNamedPipe. Not an issue, just worth mentioning the small difference.

Author:	carlossanlop
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[ericstj]

We should push PipeAccessRights down from System.IO.Pipes.AccessControl to System.IO.Pipes and add the NamedPipeClientStream constructor. This type is a simple enum so it should be easy to push down. I think it was mistakenly identified as only useful for ACLs when it's actually like the Pipe version of the FileAccess enum.
Probably we need to consider the linux behavior of this (ignore or throw) since I bet these flags are probably Windows specific.

[kenans]

Given #94742 is closed, wondering if there's any update on this one?

Please note that this is also blocking scenarios such as modifying DACLs from a NamedPipeClientStream via PipesAclExtensions.SetAccessControl(PipeStream, PipeSecurity), which is a legit operation in .NET Framework.

The reason for that is flags such as PipeAccessRights.ChangePermissions or PipeAccessRights.FullControl can't be given due to a lack of the NamedPipeClientStream constructor, as mentioned in @ericstj 's comment, leading to a constant UnauthorizedAccessException when updating DACLs from a client stream.

[ericstj]

I think it's marked as api-needs-work just because it's missing the template. Probably the easiest thing to do here is format this as an API request to get it approved and then submit the PR. I can help with that.

[bartonjs]

Video

The desired functionality should already be possible with NamedPipeServerStreamAcl.Create, but apparently there's a bug.

Rather than adding this constructor, we should either fix the bug or bring back all of the constructors that were cut for being Windows-only (merging the Pipes and Pipes.AccessControl assembly implementations).

[carlossanlop]

I opened #98060 to follow up on fixing the bug.

[ericstj]

I think there was a misunderstanding (perhaps from a typo I corrected). This issue requests an API for NamedPipeClientStream - not NamedPipeServerStream. There is no corresponding Create API to fix - we don't have one. As I mentioned above - adding the Create method was an alternative, but I don't think that's worth it since we can do better by just exposing the constructor.

I think moving all the ACLs APIs back is an interesting follow on issue, but I think we should fix this problem first since it's distinct from ACLs.

[kevincathcart-cas]

To follow up on a few things questioned in the API review stream:

1. Both assemblies in question already share a single implementation assembly on Windows. (The runtime version of System.IO.Pipes.AccessControl.dll on Windows consists entirely of TypeForwardedTos to System.IO.Pipe.dll). It is only on other platforms where the autogenerated PlatformNotSupported stubs live in System.IO.Pipes.AccessControl.dll.

2. As far as the Client Stream, this one constructor is the only one missing vs .NET framework. Indeed with that added, As far as I can tell be NamedPipeClientStream would have full source compatibility with .NET Framework on Windows if System.IO.Pipes.AccessControl is referenced. To get full source compatibility on windows for all of Pipes, would need some similar constructors added to for the Server Streams. (Please note I am saying source compatibility, not API compatibility. This is because two missing methods in PipeStream get handled by extension methods.)

3. Getting full Framework API (and ABI) compatibility for Pipe Streams is a bit more work. PipeStream would need to add these two APIs.

[SupportedOSPlatform("windows")]
public PipeSecurity GetAccessControl();
[SupportedOSPlatform("windows")]
public void SetAccessControl(PipeSecurity pipeSecurity);

The Windows implementations already exist as extension methods that literally be copied to the class. (They are only like 3-5 lines each). Other platforms would just throw PlatformNotSupported exceptions, obviously.

The hard thing would be that PipeSecurity and friends would need to move reference assembly, and I'm unsure if using the autogenerated PlatformNotSupported stubs would still be possible, or if handwritten stubs would be needed. This would also imply having the System.IO.Pipes reference System.Security.AccessControl.

---

My suggestion:

Add this constructor and the equivalent ones for the Server Streams. That seem like it gives full source compat when targeting Windows and referencing System.Security.AccessControl. Which I think could win over some of the API reviewers.

A later effort could look fully merging the two assemblies, to allow those two missing APIs to be added, enabling full API and ABI compat. (With System.Security.AccessControl becoming just a facade in both Reference and runtime assembly form.)

Hope this helps.

[bartonjs]

Video

Looks good as proposed. We discussed whether PipeAccessRights needed SupportedOSPlatform("windows"), and decided that "no" was the correct answer.

The nullability annotations should be made correct based on what .NET Framework accepts/rejects.

namespace System.IO.Pipes;

// existing type
public sealed partial class NamedPipeClientStream : PipeStream
{
    // new constructor
    [SupportedOSPlatform("windows")]
    public NamedPipeClientStream(String serverName, String pipeName, PipeAccessRights desiredAccessRights,
           PipeOptions options, TokenImpersonationLevel impersonationLevel, HandleInheritability inheritability)
}

// existing type moved from System.IO.Pipes.AccessControl assembly.
[System.FlagsAttribute]
public enum PipeAccessRights
{
    ReadData = 1,
    WriteData = 2,
    CreateNewInstance = 4,
    ReadExtendedAttributes = 8,
    WriteExtendedAttributes = 16,
    ReadAttributes = 128,
    WriteAttributes = 256,
    Write = 274,
    Delete = 65536,
    ReadPermissions = 131072,
    Read = 131209,
    ReadWrite = 131483,
    ChangePermissions = 262144,
    TakeOwnership = 524288,
    Synchronize = 1048576,
    FullControl = 2032031,
    AccessSystemSecurity = 16777216,
}