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

NamedPipeClientStream Connect can use uninitialized _normalizedPipePath #32760

Open
eerhardt opened this issue Feb 24, 2020 · 1 comment
Open
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@eerhardt
Copy link
Member

When adding nullable annotations for System.Net.Sockets (#32675), it was discovered that there is a scenario when using NamedPipeClientStream can use a null string:

When calling NamedPipeClientStream.Connect on Unix, it will try using the _normalizedPipePath string:

private bool TryConnect(int timeout, CancellationToken cancellationToken)
{
// timeout and cancellationToken aren't used as Connect will be very fast,
// either succeeding immediately if the server is listening or failing
// immediately if it isn't. The only delay will be between the time the server
// has called Bind and Listen, with the latter immediately following the former.
var socket = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified);
SafePipeHandle? clientHandle = null;
try
{
socket.Connect(new UnixDomainSocketEndPoint(_normalizedPipePath));

However, if the NamedPipeClientStream object was created with the constructor that takes an existing SafePipeHandle and isConnected: false, then _normalizedPipePath will be null.

public NamedPipeClientStream(PipeDirection direction, bool isAsync, bool isConnected, SafePipeHandle safePipeHandle)
: base(direction, 0)
{

Doing some searching, I'm honestly not certain how this scenario (existing SafePipeHandle and isConnected: false) can ever work successfully. It will need a path to be able to connect to, which it wasn't given. So at best we may be able to throw a better exception than the ArgumentNullException we get:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'path')
   at System.Net.Sockets.UnixDomainSocketEndPoint..ctor(String path)
   at System.IO.Pipes.NamedPipeClientStream.TryConnect(Int32 timeout, CancellationToken cancellationToken)
   at System.IO.Pipes.NamedPipeClientStream.ConnectInternal(Int32 timeout, CancellationToken cancellationToken, Int32 startTime)
   at System.IO.Pipes.NamedPipeClientStream.Connect(Int32 timeout)
   at System.IO.Pipes.NamedPipeClientStream.Connect()

/cc @stephentoub

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Feb 24, 2020
@JeremyKuhne JeremyKuhne added this to the 5.0 milestone Mar 5, 2020
@JeremyKuhne JeremyKuhne added bug help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Mar 5, 2020
@carlossanlop
Copy link
Member

I also noticed we do not have any NamedPipeClientStream unit tests verifying isConnected=false.

I wonder if it's possible to retrieve the path of a named pipe from only a SafePipeHandle. If this is not possible, then I think the exception should be thrown directly in the constructor if isConnected=false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants