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

System.IO.Pipes.PipeStream no-op's writing 0 bytes #81053

Open
mconnew opened this issue Jan 23, 2023 · 16 comments
Open

System.IO.Pipes.PipeStream no-op's writing 0 bytes #81053

mconnew opened this issue Jan 23, 2023 · 16 comments
Milestone

Comments

@mconnew
Copy link
Member

mconnew commented Jan 23, 2023

Description

There is no direct mechanism to know the difference between the remote end of a named pipe aborting the connection or closing it cleanly. When using message transmission mode with named pipes on Windows, it's possible to send a 0 byte message. The receiver receives a 0 byte payload on a receive call. WCF on .NET Framework uses this to tell the difference between a close and an abort. WCF does a 0 byte handshake where one end notifies the other end that it is initiating a clean shutdown by sending a 0 byte payload. It then waits for the other end to also send a 0 byte payload. Both ends then close the named pipe handle.

In porting the WCF NamedPipe transport, there is a need to send a 0 byte payload for this close handshake. PipeStream.WriteAsync checks if a zero byte buffer is attempting to be sent, and if so returns Task.CompletedTask.

Reproduction Steps

Establish a PipeStream between a client and server. One end calls await PipeStream.WriteAsync(new byte[0], 0, 0);'. The other end calls int bytes = await PipeStream.ReadAsync();` and continues waiting indefinitely as it never receives the 0 byte payload.

Expected behavior

WriteAsync should send a 0 byte payload and the receiver should complete the ReadAsync with 0 bytes received.

Actual behavior

WriteAsync is a no-op and the ReadAsync waits indefinitely.

Regression?

No, this is the behavior of PipeStream on .NET Framework and all earlier versions. I'm raising this as an issue as I'm trying to avoid any P/Invoke calls in the WCF/CoreWCF codebase and the managed wrapper doesn't have full fidelity of the native win32 api's.

Known Workarounds

The only workaround is to create the named pipe using win32 api's and construct a NamedPipeServerStream or NamedPipeClientStream from the native handle, then when I want to send a 0 byte message, P/Invoke win32 api's to send it.

Configuration

No response

Other information

This request in change of behavior might need a new API to opt-in or opt-out as it might break some users if they send 0 bytes resulting in the receiver getting 0 bytes returned from ReceiveAsync and interpreting it as an EOF response.

WCF/CoreWCF don't need this capability in Unix as we aren't going to support NamedPipe's in Unix.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 23, 2023
@ghost
Copy link

ghost commented Jan 23, 2023

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

Issue Details

Description

There is no direct mechanism to know the difference between the remote end of a named pipe aborting the connection or closing it cleanly. When using message transmission mode with named pipes on Windows, it's possible to send a 0 byte message. The receiver receives a 0 byte payload on a receive call. WCF on .NET Framework uses this to tell the difference between a close and an abort. WCF does a 0 byte handshake where one end notifies the other end that it is initiating a clean shutdown by sending a 0 byte payload. It then waits for the other end to also send a 0 byte payload. Both ends then close the named pipe handle.

In porting the WCF NamedPipe transport, there is a need to send a 0 byte payload for this close handshake. PipeStream.WriteAsync checks if a zero byte buffer is attempting to be sent, and if so returns Task.CompletedTask.

Reproduction Steps

Establish a PipeStream between a client and server. One end calls await PipeStream.WriteAsync(new byte[0], 0, 0);'. The other end calls int bytes = await PipeStream.ReadAsync();` and continues waiting indefinitely as it never receives the 0 byte payload.

Expected behavior

WriteAsync should send a 0 byte payload and the receiver should complete the ReadAsync with 0 bytes received.

Actual behavior

WriteAsync is a no-op and the ReadAsync waits indefinitely.

Regression?

No, this is the behavior of PipeStream on .NET Framework and all earlier versions. I'm raising this as an issue as I'm trying to avoid any P/Invoke calls in the WCF/CoreWCF codebase and the managed wrapper doesn't have full fidelity of the native win32 api's.

Known Workarounds

The only workaround is to create the named pipe using win32 api's and construct a NamedPipeServerStream or NamedPipeClientStream from the native handle, then when I want to send a 0 byte message, P/Invoke win32 api's to send it.

Configuration

No response

Other information

This request in change of behavior might need a new API to opt-in or opt-out as it might break some users if they send 0 bytes resulting in the receiver getting 0 bytes returned from ReceiveAsync and interpreting it as an EOF response.

WCF/CoreWCF don't need this capability in Unix as we aren't going to support NamedPipe's in Unix.

Author: mconnew
Assignees: -
Labels:

area-System.IO

Milestone: -

@adamsitnik
Copy link
Member

Hi @mconnew

Big thanks for providing very detailed and thoughtful description.

There is no direct mechanism to know the difference between the remote end of a named pipe aborting the connection or closing it cleanly.

I wonder if calling GetLastError right after ReadAsync that returned 0 bytes could work as we should be still running on the same thread in case of these errors:

// EOF on a pipe. Callback will not be called.
// We clear the overlapped status bit for this special case (failure
// to do so looks like we are freeing a pending overlapped later).

In theory it would be a matter of handling following error codes:

internal static bool IsEndOfFile(int errorCode, SafeFileHandle handle, long fileOffset)
{
switch (errorCode)
{
case Interop.Errors.ERROR_HANDLE_EOF: // logically success with 0 bytes read (read at end of file)
case Interop.Errors.ERROR_BROKEN_PIPE: // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe.
case Interop.Errors.ERROR_PIPE_NOT_CONNECTED: // Named pipe server has disconnected, return 0 to match NamedPipeClientStream behaviour
case Interop.Errors.ERROR_INVALID_PARAMETER when IsEndOfFileForNoBuffering(handle, fileOffset):
return true;
default:
return false;
}
}

But I've not tested it and I am not pipes expert yet so I am not 100% sure. We could use these tests to verify that.

This request in change of behavior might need a new API

We could definitely extend PipeOptions with a new value like "PerformZeroByteWrites" (or any other better name). It would be the cleanest solution, but available only in .NET 8 (we could aim for Preview 2). Would that be acceptable?

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jan 24, 2023
@adamsitnik adamsitnik self-assigned this Jan 24, 2023
@mconnew
Copy link
Member Author

mconnew commented Jan 25, 2023

The problem with adopting a new scheme is that we need to interop with the existing WCF implementation in .NET Framework. A new pipe option would work, but it wouldn't be adopted for a few years as CoreWCF doesn't require the latest runtime. As an enum is just an int, you can cast any arbitrary int to an enum. Would backporting without adding the value to the enum be an option? Currently only high bits are used for PipeOptions, so lets say PerformZeroByteWrites has a value of 1. Then I could do PipeOptions myPipeOptions = (PipeOptions)1; to pass the value.

@adamsitnik
Copy link
Member

adamsitnik commented Jan 25, 2023

Would backporting without adding the value to the enum be an option?

I could try to convince others to backport to 7.0 and 6.0, but it would be an incomplete solution, as CoreWCF targets .NET Standard 2.0 which can be used by .NET Core 2.1, 3.1 and 5.0 (not supported anymore) and Framework (only security fixes allowed).

Another alternative that I've considered was creating NamedPipeServerStream or NamedPipeClientStream, connecting and then creating FileStream out of the exposed pipe handles:

async Task<(SafeFileHandle readHandle, SafeFileHandle writeHandle)> GetNamedPipeHandlesAsync()
{
    string name = FileSystemTest.GetNamedPipeServerStreamName();

    var server = new NamedPipeServerStream(name, PipeDirection.In, -1, PipeTransmissionMode.Byte, PipeOptions);
    var client = new NamedPipeClientStream(".", name, PipeDirection.Out, PipeOptions);

    await Task.WhenAll(server.WaitForConnectionAsync(), client.ConnectAsync());

    bool isAsync = (PipeOptions & PipeOptions.Asynchronous) != 0;
    return (GetFileHandle(server, isAsync), GetFileHandle(client, isAsync));
}

private static SafeFileHandle GetFileHandle(PipeStream pipeStream, bool isAsync)
{
    var serverHandle = new SafeFileHandle(pipeStream.SafePipeHandle.DangerousGetHandle(), ownsHandle: true);
    pipeStream.SafePipeHandle.SetHandleAsInvalid();
    return serverHandle;
}

It would work for sync IO handles, but not for async IO handles due to #28585 (comment)

The only workaround is to create the named pipe using win32 api's and construct a NamedPipeServerStream or NamedPipeClientStream from the native handle, then when I want to send a 0 byte message, P/Invoke win32 api's to send it.

One more workaround idea is to create types that derive from NamedPipeServerStream and NamedPipeClientStream, override all Read/Write operations and perform ReadFile and WriteFile sys-calls for the 0-byte long buffers.

@adamsitnik
Copy link
Member

@stephentoub I've run out of ideas, do you have any suggestions?

@stephentoub
Copy link
Member

stephentoub commented Jan 25, 2023

I'm not particularly keen on the idea of exposing any ability for a Stream.Read{Async} implementation to return 0 when a non-0 length has been provided and it's not EOF. That aspect of the design has always been very strict, and even if this is exposed as an option, the stream could be handed out to an unsuspecting consumer. It sounds like in theory this is already possible today via interop to create the native handles and interop to send the message, such that a NamedPipeClientStream constructed from that would then potentially receive a 0-byte message, but that's a lot more hoops to jump through than making it trivial via some new option (and already involves interop, at which point lots of bets are off).

Let's revisit the original problem statement: "I'm trying to avoid any P/Invoke calls in the WCF/CoreWCF codebase". I completely get the goal of driving that down, that's a great goal. But getting it to 0... in any large codebase that's previously done a lot of interop and that's inherited a lot of Windows-specific behaviors, I don't think we need to wrap every single thing in the .NET core libraries to make that possible. I think we need to expect and be ok with there still being the odd P/Invoke on this or that platform to be able to achieve other desired goals around functionality and compatibility, especially when we're talking about interoperating with older versions running on .NET Framework and those systems have those Windows-isms ingrained.

@mconnew
Copy link
Member Author

mconnew commented Jan 25, 2023

Today PipeStream.Read{Async} can return 0 when it's not EOF as the ability to send a 0 byte message is a fundamental capability of the win32 api's. You could have a client app which only uses NamedPipeClientStream in a vanilla way with zero interop code and it can receive a 0 byte message as the other end of the pipe doesn't have to be NamedPipeServerStream. This is the current situation with the new System.ServiceModel.NamedPipe preview package which uses NamedPipeClientStream in a vanilla way. Not supporting sending 0 bytes doesn't prevent receiving 0 bytes without EOF. I don't think this is a valid argument against this as nothing changes other than you would be able to send 0 bytes with only managed code too.

We are getting away with not having this feature on the client as the way the close protocol works, as long as one end sends the 0 byte message, we're fine. The problem comes when both ends don't actually send the 0 byte message.

The motivation behind aggressively avoiding P/Invoke calls in the WCF client codebase is there isn't the developer resources to maintain a cross platform interop implementation which works on Windows, Linux, and Mac. This is the only place in WCF where that doesn't apply as this is our first feature that is Windows only.
I believe it's also a good forcing function to push for capabilities in the runtime which others can take advantage of. I'm also a big believer in not having multiple implementations of the same capability. It doubles the chance of security issues, requires double the performance improvement investment etc. For example, a recent change in SslStream that I requested will improve performance for anyone in a domain. If we had our own implementation, I could have fixed it in our code, and we'd be the only ones who benefit.

@stephentoub
Copy link
Member

This is the current situation with the new System.ServiceModel.NamedPipe preview package which uses NamedPipeClientStream in a vanilla way.

I said as much in my response. My point was that it's not possible today via the .NET APIs alone: there's no way to create a NamedPipeServerStream that would trigger this, correct me if I'm wrong. I'm loathe to double-down on breaking the Stream contract by exposing the ability to do so via .NET APIs. And I do believe that's a valid argument.

@bartonjs
Copy link
Member

I'm a bit confused here.

If the existing implementation of the pipe stream will return 0 from Read/ReadAsync provided that a writer performed a zero-byte write, that seems like a bug that we should just fix... since the writer need not be a .NET application. (Right?) I don't see (maybe I didn't read hard enough) a request to have Read reliably return 0 just because a writer wrote 0.

Then if the issue is just that the writer has an "if (length == 0) return;", it seems like this issue is just asking for that guard to be removed. Since it would potentially exacerbate the Read-returns-zero problem they should be fixed concurrently (or Read-returns-zero could be fixed first).

@bartonjs
Copy link
Member

Nevermind, I see where it did request reliably reading zero: "the receiver should complete the ReadAsync with 0 bytes received."

@mconnew
Copy link
Member Author

mconnew commented Jan 25, 2023

I think the question comes down to whether it's intended for the managed named pipe implementations to be capable of communicating with any process at the other end of the named pipe including those which aren't using the .NET managed wrapper, or if it's scoped to only communicate with .NET at the other end.

If it's only scoped to communicate with .NET at the other end, we should document that. If it's intended to communicate with non .NET (or .NET using win32 api's more directly), then this is a gap in capability as it is insufficient to communicate with some other processes.

@stephentoub
Copy link
Member

If it's only scoped to communicate with .NET at the other end, we should document that.

It's not scoped to only communicate with .NET, but that doesn't mean it's supportive of every single possible thing at the other end. There are things in the middle of that spectrum.

In this case, from my perspective, it's a bug that Stream.Read{Async} ever returns 0 when non-0 is requested and not at EOF. Things at the other end of the pipe that would lead to triggering that situation are, from my perspective, part of the unsupported middle.

@mconnew
Copy link
Member Author

mconnew commented Jan 25, 2023

In the specific WCF usage scenario, we are assigning a semantic meaning to sending 0 bytes to be the equivalent of Socket.Shutdown(SocketShutdown.Send). Not allowing receiving any more bytes after receiving a 0 byte message would work for the WCF scenario, and maintains the concept of returning 0 bytes means there's nothing else to follow. I believe this solves the problem for WCF while maintaining generic Stream api semantics.

@mconnew
Copy link
Member Author

mconnew commented Jan 26, 2023

@stephentoub, regardless of the result of this discussion, if there's a gap between what's possible with native api calls and with PipeStream, that should be documented.

@stephentoub
Copy link
Member

if there's a gap between what's possible with native api calls and with PipeStream, that should be documented

You just mean documenting explicitly that doing a 0-byte write on a PipeStream is a nop?

@mconnew
Copy link
Member Author

mconnew commented Jan 28, 2023

If this is a won't fix, then yes. Also the behavior around when receiving a 0 byte message if the behavior is modified as a result of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants