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

[API Proposal]: Add PipeOptions.FirstPipeInstance enum value #76984

Closed
JamesNK opened this issue Oct 13, 2022 · 9 comments · Fixed by #83936
Closed

[API Proposal]: Add PipeOptions.FirstPipeInstance enum value #76984

JamesNK opened this issue Oct 13, 2022 · 9 comments · Fixed by #83936
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking-release partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 13, 2022

Background and motivation

It's not possible for an app hosting multiple named pipe connections to verify that no one else is using the pipe name when it starts.

For example, Kestrel is looking at adding a named pipe transport, and when we start Kestrel we want to verify that no one else is using the configured pipe name:

  • We don't want to share the pipe between Kestrel instances. It's common for devs to accidentally launch Kestrel when it's already running. A socket errors if you bind to it when it's already open. We want the same protection with a named pipe.
  • A pipe gets many of its settings when it is first opened. Want to verify that Kestrel's settings are applied to the pipe.

Named pipes have a built-in option for checking this - FILE_FLAG_FIRST_PIPE_INSTANCE. See https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea

.NET doesn't expose this option. While it is automatically set if the max number of pipe instances is 1, there is no way to explicitly set this option.

int openMode = ((int)direction) |
(maxNumberOfServerInstances == 1 ? Interop.Kernel32.FileOperations.FILE_FLAG_FIRST_PIPE_INSTANCE : 0) |
(int)options |
(int)additionalAccessRights;
.

If the PipeOptions.FirstPipeInstance options was available, then Kestrel would create the first NamedPipeServerStream with it the option to verify that there is no existing pipe with that name and create subsequent NamedPipeServerStream instances without the option.

Note that validation on the NamedPipeServerStream ctor currently prevents the enum value having FILE_FLAG_FIRST_PIPE_INSTANCE value added to it. e.g.

// NamedPipeServerStream errors when passed this pipe option
pipeOptions |= (PipeOptions)0x00080000;

API Proposal

namespace System.IO.Pipes
{
    [Flags]
    public enum PipeOptions
    {
        FirstPipeInstance = unchecked((int)0x00080000)
    }
}

API Usage

private NamedPipeServerStream CreateServerStream(bool firstStream)
{
    var pipeOptions = PipeOptions.Asynchronous | PipeOptions.WriteThrough;
    if (_options.CurrentUserOnly)
    {
        pipeOptions |= PipeOptions.CurrentUserOnly;
    }
    if (firstStream)
    {
        pipeOptions |= PipeOptions.FirstPipeInstance;
    }

    return new NamedPipeServerStream(
        _endpoint.PipeName,
        PipeDirection.InOut,
        NamedPipeServerStream.MaxAllowedServerInstances,
        PipeTransmissionMode.Byte,
        pipeOptions,
        inBufferSize: 0,
        outBufferSize: 0);
}

Alternative Designs

[Flags]
public enum PipeOptions
{
    EnsureFirstPipeInstance = unchecked((int)0x00080000)
}

It still alludes at the name of the Windows API flag and conveys "throw if already exists".

Risks

No response

@JamesNK JamesNK added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 13, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

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

Issue Details

Background and motivation

It's not possible for an app hosting multiple named pipe connections to verify that no one else is using the pipe name when it starts.

For example, Kestrel is looking at adding a named pipe transport, and when we start Kestrel we want to verify that no one else is using the configured pipe name.

Named pipes have a built-in option for checking this - FILE_FLAG_FIRST_PIPE_INSTANCE. See https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea

.NET doesn't expose this option. While it is automatically set if the max number of pipe instances is 1, there is no way to explicitly set this option.

int openMode = ((int)direction) |
(maxNumberOfServerInstances == 1 ? Interop.Kernel32.FileOperations.FILE_FLAG_FIRST_PIPE_INSTANCE : 0) |
(int)options |
(int)additionalAccessRights;
.

If the PipeOptions.FirstPipeInstance options was available, then Kestrel would create the first NamedPipeServerStream with it the option to verify that there is no existing pipe with that name and create subsequent NamedPipeServerStream instances without the option.

Note that validation on the NamedPipeServerStream ctor currently prevents the enum value having FILE_FLAG_FIRST_PIPE_INSTANCE value added to it. e.g.

// NamedPipeServerStream errors when passed this pipe option
pipeOptions |= (PipeOptions)0x00080000;

API Proposal

namespace System.IO.Pipes
{
    [Flags]
    public enum PipeOptions
    {
        FirstPipeInstance = unchecked((int)0x00080000)
    }
}

API Usage

private NamedPipeServerStream CreateServerStream(bool firstStream)
{
    var pipeOptions = PipeOptions.Asynchronous | PipeOptions.WriteThrough;
    if (_options.CurrentUserOnly)
    {
        pipeOptions |= PipeOptions.CurrentUserOnly;
    }
    if (firstStream)
    {
        pipeOptions |= PipeOptions.FirstPipeInstance;
    }

    return new NamedPipeServerStream(
        _endpoint.PipeName,
        PipeDirection.InOut,
        NamedPipeServerStream.MaxAllowedServerInstances,
        PipeTransmissionMode.Byte,
        pipeOptions,
        inBufferSize: 0,
        outBufferSize: 0);
}

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-System.IO, untriaged

Milestone: -

@jozkee
Copy link
Member

jozkee commented Nov 29, 2022

Thanks for the proposal, it looks to me like a good case to expose the option.

Note that validation on the NamedPipeServerStream ctor currently prevents the enum value having

I also thought of just excluding FILE_FLAG_FIRST_PIPE_INSTANCE from said validation, but the history shows that it is in place since Pipe code was ported over to core.

Regarding the proposed name, I'm not sure if FirstPipeInstance is the best name, since it doesn't quite convey that it will throw if the pipe already exists.

As an alternative proposal I suggest EnsureFirstPipeInstance, it still alludes at the Windows flag and conveys "error if already exists".

@jozkee jozkee added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Nov 29, 2022
@jozkee jozkee added this to the 8.0.0 milestone Nov 29, 2022
@JamesNK
Copy link
Member Author

JamesNK commented Nov 29, 2022

I went with FirstPipeInstance because it lines up with the windows API flag. I agree that something like EnsureFirstPipeInstance is more descriptive.

@terrajobst
Copy link
Member

terrajobst commented Nov 29, 2022

Video

  • The proposal looks reasonable, but given the angle is security, we can't just no-op this in the non-Windows implementation. Is there a reasonable cross-platform implementation or do we need to consider this a Windows-only feature?
    • If it's Windows-only we should mark the enum member as [SupportedOSPlatform("windows")] and throw PlatformNotSupportedException from the corresponding methods using it (i.e. the constructors)
namespace System.IO.Pipes
{
    [Flags]
    public enum PipeOptions
    {
        FirstPipeInstance = unchecked((int)0x00080000)
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 29, 2022
@JamesNK
Copy link
Member Author

JamesNK commented Jan 25, 2023

Named pipes support in Kestrel is limited to Windows only. Ideally, PipeOptions.FirstPipeInstance would be cross-platform, but if isn't possible on Linux/macOS then limiting it to Windows is fine with me.

@Tarun047
Copy link
Contributor

Hi People, I'd like to take this up and contribute a PR.

@jozkee
Copy link
Member

jozkee commented Mar 24, 2023

@Tarun047 that would be very helpful. Please feel free to do so.

@Tarun047
Copy link
Contributor

Tarun047 commented Mar 25, 2023

I've created a commit in my fork

I wanted to make sure these are the changes that we should be doing before creating a PR and I don't want to create a
PR that fails during build.

When I build my local repository with the following command
./build.sh -c Release -subset clr+libs+libs.tests

it's throwing a build error which says the call site is reachable on Unix but the parameter PipeOptions.FirstPipeInstance itself is supported only on Windows.
Screenshot 2023-03-25 at 12 07 52 PM

But I don't see why this is a problem, because even PipeTransmissionMode.Message is also annotated as only supported by Windows, but that doesn't throw any build error.

I would really appreciate any help in this regard and let me know if I'm missing something here.

@jozkee
Copy link
Member

jozkee commented Apr 3, 2023

@Tarun047 that seems to me like a bug in CA1416 analyzer. I just filed an issue for it #84258.

@jeffhandley jeffhandley added partner-impact This issue impacts a partner who needs to be kept updated blocking-release labels May 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO blocking-release partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants