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.

Sign up for GitHub

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

Jump to bottom

NamedPipeClientStream Connect can use uninitialized _normalizedPipePath #32760

Open
eerhardt opened this issue Feb 24, 2020 · 2 comments · May be fixed by #110075
Open

NamedPipeClientStream Connect can use uninitialized _normalizedPipePath #32760

eerhardt opened this issue Feb 24, 2020 · 2 comments · May be fixed by #110075
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Milestone
Future

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:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.Unix.cs

Lines 21 to 31 in 032e8db

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.

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs

Lines 101 to 103 in 032e8db

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
@eerhardt eerhardt mentioned this issue Feb 26, 2020
Merged
@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.

@carlossanlop carlossanlop mentioned this issue Mar 13, 2020
Merged
@carlossanlop carlossanlop modified the milestones: 5.0.0, Future Jun 18, 2020
@hongman hongman mentioned this issue Aug 8, 2020
Closed
@koenigst
Copy link
Contributor

I did some digging. The combination of passing a SafePipeHandle and isConnected=false just does not make sense neither on Unix nor on Windows. The only reason why it was not flagged on Windows is because the lpFileName argument of CreateFileW is marked as nullable (it is non-nullable in other places).

Also if isConnected=false then the passed SafePipeHandle will be overwritten when connecting which is probably not what the user wanted.

I will add an ArgumentOutOfRangeException in the constructor for isConnected=false. Seems weird to have that argument at all. Should I mark the constructor as obsolete and create a replacement?

koenigst added a commit to koenigst/runtime that referenced this issue Nov 22, 2024
@koenigst
Added an argument check for isConnected.
c66e738 
* Check that isConnected in the NamedPipeClientStream constructor is always true.
* Added an argument test.

Fixes dotnet#32760
koenigst added a commit to koenigst/runtime that referenced this issue Nov 22, 2024
@koenigst
Removed the isConnected argument from the constructor.
5dcac88 
* Created a new constructor without the argument.
* Marked the old constructor as obsolete.

Fixes dotnet#32760
koenigst added a commit to koenigst/runtime that referenced this issue Nov 22, 2024
@koenigst
Added an argument check for isConnected.
bfecb79 
* Check that isConnected in the NamedPipeClientStream constructor is always true.
* Added an argument test.

Fixes dotnet#32760
koenigst added a commit to koenigst/runtime that referenced this issue Nov 22, 2024
@koenigst
Removed the isConnected argument from the constructor.
9c02e5f 
* Created a new constructor without the argument.
* Marked the old constructor as obsolete.

Fixes dotnet#32760
@koenigst koenigst linked a pull request Nov 22, 2024 that will close this issue
Open
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-System.IO bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Milestone
Future
Development

Successfully merging a pull request may close this issue.

NamedPipeClientStream Connect with SafePipeHandle koenigst/runtime
5 participants
@carlossanlop @koenigst @JeremyKuhne @eerhardt @Dotnet-GitSync-Bot