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]: Process constructor from SafeProcessHandle #71595

Open
NN--- opened this issue Jul 3, 2022 · 7 comments
Open

[API Proposal]: Process constructor from SafeProcessHandle #71595

NN--- opened this issue Jul 3, 2022 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@NN---
Copy link
Contributor

NN--- commented Jul 3, 2022

Background and motivation

There are plenty of ways to create a process where .NET does not support this or where there is already a process handle from native method.
Currently there is no possibility to create a .NET Process object from the SafeProcessHandle.

API Proposal

namespace System.Diagnostics;

public class Process
{
 public Process(SafeProcessHandle process)
}

API Usage

using SafeProcessHandle processHandle = Win32Api.CreateProcess();
Process process = new Process(processHandle);

Alternative Designs

Adding per platform different process creation options.
Such as #71515

Risks

There should be no risk.
We already have a precedent with Socket constructor from SafeSockegHandle.

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

ghost commented Jul 3, 2022

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

Issue Details

Background and motivation

There are plenty of ways to create a process where .NET does not support this or where there is already a process handle from native method.
Currently there is no possibility to create a .NET Process object from the SafeProcessHandle.

API Proposal

namespace System.Diagnostics;

public class Process
{
 public Process(SafeProcessHandle process)
}

API Usage

using SafeProcessHandle processHandle = Win32Api.CreateProcess();
Process process = new Process(processHandle);

Alternative Designs

No response

Risks

There should be no risk.
We already have a precedent with Socket constructor from SafeSockegHandle.

Author: NN---
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics.Process

Milestone: -

@KalleOlaviNiemitalo
Copy link

Would this be a Windows-only API? Not clear to me how you'd get a SafeProcessHandle from a native method on Linux or Mac.

@AraHaan
Copy link
Member

AraHaan commented Jul 3, 2022

I am sure there is a way yes since they too have process ID's.

@DaZombieKiller
Copy link
Contributor

Would this be a Windows-only API? Not clear to me how you'd get a SafeProcessHandle from a native method on Linux or Mac.

On non-Windows, Process.SafeHandle will return a pseudo-SafeProcessHandle wrapping the process ID, but there's no way to get the wrapped process ID from the handle, making the pseudo-handle almost useless since you can't get back to the Process. I don't think you can get such an instance from a P/Invoke, though I've never tried.

@fbrosseau
Copy link

fbrosseau commented Jul 4, 2022

Would this be a Windows-only API? Not clear to me how you'd get a SafeProcessHandle from a native method on Linux or Mac.

Modern linux does have proper process file descriptors, but that feature may be too modern for dotnet support (I don't know?). They do solve many problems however - race conditions around PID recycling, etc.

@DaZombieKiller
Copy link
Contributor

Thinking about this further, would it make more sense for this to be exposed as a static method like GetProcessById?

using Microsoft.Win32.SafeHandles;

namespace System.Diagnostics;

public partial class Process
{
    public static Process GetProcessByHandle(SafeProcessHandle handle);
    public static Process GetProcessByHandle(IntPtr handle); // potential extra API
}

Having a constructor would be consistent with Socket, but the static method is more consistent with the other APIs on Process. It'd also open up the opportunity for the Process objects to be cached, though I don't believe they are currently, even for GetProcessById.

@KalleOlaviNiemitalo
Copy link

For creating a Process from a native process handle (SafeProcessHandle or IntPtr) on Windows, I don't think caching would be a good idea. The handle might not have the same access rights as what the Process class specifies when it opens a process handle, and it might have access to a process that the current process is not normally authorised to open. If I created a Process object from such a handle, I would not want that to affect the behaviour of future Process.GetProcessById calls because such an effect would make my app more difficult to test. Also, I would not want Process.Dispose of a different process object to close my process handle prematurely.

Re implementing this on Linux, pidfd_open seems to be the function referenced in #71595 (comment). If I read this correctly, a pidfd does not carry any access rights (if you receive a pidfd, you cannot use that to kill a process that you wouldn't otherwise be authorised to kill).

@Jozkee Jozkee added this to the Future milestone Jul 19, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 19, 2022
@Jozkee Jozkee added untriaged New issue has not been triaged by the area owner needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jul 19, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

6 participants