Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed setting ReadMode to Message for NamedPipeClientStream on Windows #94742

Closed

Conversation

RogerSanders
Copy link

Fixes a longstanding issue inherited from the .NET framework which prevents ReadMode being set to Message on the client end of read only named pipes on Windows platforms.

Also added in new tests for Message mode on Windows to verify correct functionality.

Fix #83072

Fixes a longstanding issue inherited from the .NET framework which
prevents ReadMode being set to Message on the client end of read only
named pipes on Windows platforms.

Also added in new tests for Message mode on Windows to verify correct
functionality.

Fix dotnet#83072
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 15, 2023
@ghost
Copy link

ghost commented Nov 15, 2023

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

Issue Details

Fixes a longstanding issue inherited from the .NET framework which prevents ReadMode being set to Message on the client end of read only named pipes on Windows platforms.

Also added in new tests for Message mode on Windows to verify correct functionality.

Fix #83072

Author: RogerSanders
Assignees: -
Labels:

area-System.IO

Milestone: -

@RogerSanders
Copy link
Author

RogerSanders commented Nov 15, 2023

Adding some comments here to explain what's happening. I recommend reading this stackoverflow post:
https://stackoverflow.com/questions/32739224/c-sharp-unauthorizedaccessexception-when-enabling-messagemode-for-read-only-name

That question/answer was posted by me back in 2015 describing a defect and a workaround for an issue with access permissions when creating named pipes in the .NET Framework. A bug report was made on Microsoft Connect, and never actioned. The same defect was inherited when .NET Core came into being, however the workaround we could do in the .NET Framework days is no longer possible, as it is no longer to manually control access permissions to work around the issue. I'm contributing the proper fix here. Areas of reference from MSDN:
https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate
https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea

See in particular this excerpt:

A client process connects to a named pipe by using the CreateFile or CallNamedPipe function. The client side of a named pipe starts out in byte mode, even if the server side is in message mode. To avoid problems receiving data, set the client side to message mode as well. To change the mode of the pipe, the pipe client must open a read-only pipe with GENERIC_READ and FILE_WRITE_ATTRIBUTES access.

This change simply brings the implementation inline with the MSDN recommendations. Since pipe message mode is Windows-specific, this doesn't affect other platforms.

@RogerSanders
Copy link
Author

@dotnet-policy-service agree

@RogerSanders
Copy link
Author

It looks like there are more unit tests to adjust which only run under "privileged" mode, but I didn't use an elevated command prompt to test so I missed them. I'll prepare a fix.

@RogerSanders
Copy link
Author

Ok, so there's a backwards compatibility issue the failing test was correct to complain about. There's no issue if the named pipe the client is trying to connect to has been created with default access permissions, and it was either created under the same user account or an administrator/system account. There's also no problem if the pipe has been created with correctly set manual permissions to support message mode pipes. By default however, other accounts on the system (IE, basic users) are only granted "Read" access to the pipe, without the "WriteAttributes" permission. Attempts to open pipes under these circumstances now fail with this change as I first submitted it. This could also occur if the pipe was created with a manual permissions set which disallowed WriteAttributes. It makes sense that any pipe a client was able to connect to prior to this change, it must also be able to connect to after it, so the original change is not suitable.

Fixing this now requires a little bit of a compromise. On the client end, we don't know whether the "WriteAttributes" permission has been granted to the current user until we try and open the pipe. The NamedPipeClientStream class also doesn't know if message mode will be requested by the caller prior to opening the pipe, as the API doesn't require the caller to provide that information. We also can't obtain this permission after opening the pipe to my knowledge, nor can we safely disconnect and reconnect with the extra permission if we discover after connecting that we do require it (IE, when the caller changes ReadMode).

The failure case introduced by the fix here is unusual, as it will ONLY occur specifically on Windows if a single-directional named pipe is created, where data flows from the server to the client, where the server and client are running under different user accounts, and the account running the client is running as an unprivileged user. That is a very narrow case.

To fix this, I propose we attempt to open the named pipe with the WriteAttributes permission as per this change by default. If that fails with a privilege issue, we drop the WriteAttributes permission and try again. This double-connect attempt would only occur internally, transparently to the caller, in the specific case where we detect this privilege issue. This means in the 99.9% case here the pipe will only be attempted to be opened once, and we will now also be able to support message mode read-only pipes from server to client, which are completely broken and not supported right now in non-framework .NET versions with no workaround.

I'll prepare another changeset with this proposed modification.

@RogerSanders
Copy link
Author

Paging @jeffhandley, as I can't see if you got pulled in correctly by the codeowners check.

@Jozkee
Copy link
Member

Jozkee commented Nov 16, 2023

@RogerSanders thanks for working on this. I think it would be best to follow the advice on #83072 (comment) to avoid granting FILE_WRITE_ATTRIBUTES by default without existing consumers of the API noticing. Would you be interested in following the steps to make the API proposal for that?

cc @ericstj.

@RogerSanders
Copy link
Author

RogerSanders commented Nov 16, 2023

Thanks for taking your time to look at this request.

Options around API changes would need more discussion. I intentionally tried to avoid making any public API changes with this fix. There's a clear issue in the API as it currently stands, as there's broken functionality with the ReadMode property, and this fix was trying to address this internally. If you want to talk about API changes, there are tradeoffs with all those that I can imagine too.

We could restore the ability to specify PipeAccessRights on a constructor to NamedPipeClientStream as proposed in #83072, however I'd point out that this would be non-sensical on non-Windows platforms, and would still be non-discoverable for the user. They'd still get errors trying to set ReadMode, with no clear indication why, and they'd have to search and find they had to use a special constructor with an arbitrary set of access rights to make it work. This would effectively leave the functionality "broken" but restore the workaround from the .NET Framework days. I wouldn't recommend this route.

There's another option. There's a constructor for NamedPipeClientStream which takes a PipeOptions flag enum. You could add a new flag like AllowMessageMode to this enum, which if set explicitly informs the constructor the caller intends to set ReadMode to Message, in which case the constructor can attempt to obtain the necessary WriteAttributes permission, failing in this case if it can't be obtained. If this was a new API with no existing usage, that would seem to be the right choice. This isn't a new API though. If you were to go with this option, no existing code in the world currently sets this flag, but message mode will still work for write-only or read/write pipes, which would also seem strange. On a new API with no existing code usage, you'd probably want to reject the attempt to set ReadMode to Message in all cases if this flag had never been passed into the constructor, which would bring the client side into line with how things work on the NamedPipeServerStream class.

So it seems there are only two viable options to fix this from my perspective:

  1. Make the WriteAttributes access right be obtained internally where possible as an implementation detail (as per this change right now). Only downside - this access right will now be requested in some cases when it was available previously, but not taken or used previously.

  2. Make the WriteAttributes access right be obtained explicitly when a new AllowMessageMode flag is set. Two downsides - the public API and documentation requires changes to reflect this, and there's an extra step to make Message mode work properly which isn't immediately apparent (passing in a special flag), which for purposes of backwards compatibility needs to work anyway if the flag hasn't been provided for write-only and read-write pipes.

Which approach would you like me to continue with? I could do another fix which implements option two, but would this one be accepted? It seems to me the third option of doing nothing and leaving the message functionality completely broken isn't viable either, so it should probably be one or the other.

Do you have any other suggestions for a clean API change that could be made here?

@Jozkee
Copy link
Member

Jozkee commented Nov 17, 2023

this would be non-sensical on non-Windows platforms

Just like PipeTransmisisonMode.Message currently is, so that argument doesn't hold IMO. What would AllowMessageMode do on non-Windows?

@Jozkee
Copy link
Member

Jozkee commented Nov 17, 2023

would still be non-discoverable for the user. They'd still get errors trying to set ReadMode, with no clear indication why, and they'd have to search and find they had to use a special constructor with an arbitrary set of access rights to make it work.

Do you think that improving the error message could address this? We could append something like "Pipes need Write or WriteAttributes access in order to change the transmission mode".

@RogerSanders
Copy link
Author

RogerSanders commented Nov 17, 2023

this would be non-sensical on non-Windows platforms

Just like PipeTransmisisonMode.Message currently is, so that argument doesn't hold IMO. What would AllowMessageMode do on non-Windows?

Emphasis on currently. Message mode isn't Windows specific, it's a logical peice of functionality which enables a form of atomicity/packets in reads/writes to a pipe. The pipe access permissions like WriteAtrributes however are highly Windows specific. I think it would be a mistake to specifically rely on exposing those attributes to the caller in order to allow them to properly enable message mode pipes. What happens if Linux or another .NET supported platform adds message mode for named pipes in the future?

This isn't as hypothetical as you may think. The pipe2 method supports the O_DIRECT flag as of kernel version 3.4, going back 10 years. That is sufficient to allow the concept of message mode to be implemented on unnamed pipes. In kernel version 4.5 (2016), it became possible to enable O_DIRECT mode for named pipes via a call to sys_fcntl. Theoretically that is enough to implement this concept. I could add a patch to implement message mode support on Linux today... but if the API changes as you propose, it would become pretty messy. To get reliable cross platform support, the caller would be required to use a Windows-specific constructor on that platform.

@RogerSanders
Copy link
Author

To be clear, if you tell me the only way forward here that will be accepted is to create a new constructor with the PipeAccessRights enum and throw a more meaningful error, I'm willing to do that. I'd like to see this issue fixed one way or the other. I'm just giving some feedback on the proposed API changes from a user perspective. Ultimately it's not my call, you let me know what you'd like to see.

@Jozkee
Copy link
Member

Jozkee commented Nov 17, 2023

Well yes, I thought that Message mode was only available on Windows only based on the [SupportedOSPlatform]. Also, I just saw an issue that we currently have no good ideas in how to implement Message mode in Unix #24097.

If you have a POC for it, can you please create a new issue? Either a blank issue or API suggestion, but it would be good to discuss it before you dive into making a full POC just to get validation/ideas from other experienced folks and if its good enough, we will incorporate it into the framework.

@Jozkee
Copy link
Member

Jozkee commented Nov 17, 2023

Also consider:

Named pipes in Unix are implemented using Sockets, not fifos, see dotnet/corefx#6833, this is due to limitations vs Windows, therefore using O_DIRECT may not be possible.

Anonymous pipes on Windows don't support Message mode, so currently there's no story of making both platforms symmetric here, however, it is fair to add support to the platform that supports it. That should be discussed in a separate issue.

Going back to my original point, if it is not possible to implement Message mode for named pipes in Unix, then it seems to me that adding the Windows-only API is the way to go since it already existed in Windows. We have prior-art of APIs that are supported only in one platform e.g: UnixFileMode. It is not a sin to do #83072 (comment).

@RogerSanders
Copy link
Author

RogerSanders commented Nov 18, 2023

Also consider:

Named pipes in Unix are implemented using Sockets, not fifos, see dotnet/corefx#6833, this is due to limitations vs Windows, therefore using O_DIRECT may not be possible.
[#83072 (comment)]

That's a surprising choice. Wow, what a mess. Since pipes are one of the primary mechanisms of IPC, it seems like a poor choice for the .NET framework to make, as you can't expect both the client and the server to be using the same language/tech stack, which may make it impossible to connect to existing systems that are built on fifos. I see an issue raised around that very point:
(dotnet/docs#36710)

The primary advantage of pipes/fifos is performance. Being little more than a thin layer over shared memory, nothing else can get close. I would have expected pipes on windows to be replicated with fifos on Unix, not sockets, they're the natural matched concept.

It's true you can't do a bi-directional fifo on Unix. You can do two unidirectional fifos however, with the "reverse connection" established after a connection request from the client. This is how we (my company) currently abstract pipes for Windows/Linux, we don't use InOut mode, we use a pair of pipes, one In one Out. If I'd been implementing this functionality in .NET core originally, I would have made InOut mode Windows-specific (which it is currently), throw on other platforms when attempting to use it, and stayed with fifos, which would have then given high performance and message mode support on Unix.

OK, I've got a more radical suggestion. Add two new flags to the API, one which says AllowMessageMode as I suggested, and another which is called.... I don't know, NoDomainSockets or ForceNativeFifo or something like that, which as it suggests, actually uses FIFOs on Unix, and can then implement message mode. If you attempt to create a pipe with InOut mode on Unix, throw an error. That fixes the problem I've described here, and also resolves the lack of fifo support on Unix, which honestly is a pretty big limitation for compatibility and integration with existing systems, as well as a major performance drawback.

Since .NET has already effectively done a language-specific implementation for domain sockets anyway on Unix, you could go a step further and implement message mode under Unix without fifos by doing some framing with a small header (IE, payload byte count) and encoding/decoding packets internally anyway - it's what we currently do for sending messages across fifos on Unix. That's walking even further into a language-specific implementation, but that horse already bolted when domain sockets were used instead of FIFOs. Maybe you need a pair of flags, ForceUnixDomainSocket and ForceUnixFifo, which if you don't specify, the implementation is an undefined, runtime specific protocol. That's basically the case now, it's just not documented that way.

@RogerSanders
Copy link
Author

Again to be clear, I have no problems opening a separate issue with an API proposal detailing this, and even a working implementation, I just figured I'd bounce a few ideas off you in this issue here and get some feedback to help craft that proposal.

@Jozkee
Copy link
Member

Jozkee commented Nov 20, 2023

I think your idea is worth of discussion with a broader forum, I suggest, again, that you file a new issue describing AllowMessageMode and ForceNativeFifo, displaying the value they would provide (perf, compatibility with other platforms, etc).

Since .NET 9 development is just starting, we have plenty of time to evaluate if that is an approach worth chasing; if not, we can fallback to move the PipeAccessRights and adding the respective ctor.

@Jozkee Jozkee closed this Nov 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set ReadMode after creating a NamedPipeClientStream instance
2 participants