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]: Kill System.Diagnostics.Process on parent death #101985

Open
elijahr2411 opened this issue May 7, 2024 · 8 comments
Open

[API Proposal]: Kill System.Diagnostics.Process on parent death #101985

elijahr2411 opened this issue May 7, 2024 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@elijahr2411
Copy link

elijahr2411 commented May 7, 2024

Background and motivation

Currently in .NET there is no way to ensure that a child process created through System.Diagnostics.Process is killed when the parent process dies, particularly unexpectedly by a SIGKILL. This proposal creates a simple property in the ProcessStartInfo class that would cause a child process to be killed by the OS in these cases.

On Linux, this could be done very easily by setting the PR_SET_PDEATHSIG flag on the fork using a prctl() syscall.

I'm not too well versed with the Windows platform but I believe something with Job Objects is possible.

I've named the property KillOnParentDeath, this can of course be changed if a better name is thought of

API Proposal

namespace System.Diagnostics;

public partial sealed class ProcessStartInfo
{
    public bool KillOnParentDeath { get; set; }
}

API Usage

// Create a process
var proc = new System.Diagnostics.Process();

// Set KillOnParentDeath
proc.StartInfo.KillOnParentDeath = true;

// If this C# program is then killed for some reason, the child will die with it instead of lingering.

Alternative Designs

No response

Risks

None as far as I'm aware. The default value should of course be false to emulate current behavior.

@elijahr2411 elijahr2411 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 7, 2024
Copy link
Contributor

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

@elijahr2411
Copy link
Author

Implements #96470

@SteveSyfuhs
Copy link

On Windows, the parent process would need to spawn the child process through a Job: https://learn.microsoft.com/en-us/windows/win32/procthread/job-objects. I've never understood why this functionality hasn't been exposed in .NET. It's certainly very useful, albeit somewhat niche -- though I think that's actually a chicken and egg problem.

@KalleOlaviNiemitalo
Copy link

Could be a property of System.Diagnostics.ProcessStartInfo instead. That way, the getter of the property would just read a field and never need any interop calls.

@KalleOlaviNiemitalo
Copy link

On Windows, JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE looks like a way to implement this. Have the parent process create a job object with that limit (and JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK?), never close the job handle, and don't make the handle inheritable. Place child processes in that job whenever KillOnParentDeath is true.

@adamsitnik
Copy link
Member

Currently, on Windows .NET is using following sys-calls to start a process:

if (!(_succeeded = Interop.Shell32.ShellExecuteExW(_executeInfo)))

retVal = Interop.Advapi32.CreateProcessWithLogonW(

Using JOB APIs makes sense, but we would need to ensure that all properties exposed by ProcessStartInfo are respected and supported when this API is used. Before we make any progress with this proposal, somebody needs to verify whether this is possible. I currently don't have any free cycles and the best I can do right now is to mark it as "help wanted" hoping that somebody will provide a working prototype. If this happens, I am going to change the API proposal to extend ProcessStartInfo with a new property and present it to the API review board.

@adamsitnik adamsitnik added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jul 17, 2024
@adamsitnik adamsitnik added this to the Future milestone Jul 17, 2024
@KalleOlaviNiemitalo
Copy link

.NET also uses CreateProcess, when ProcessStartInfo.UserName is empty:

For CreateProcessWithLogonW or CreateProcess on Windows 10, I believe the new process can be atomically made a member of a job, via STARTUPINFOEXW::lpAttributeList and UpdateProcThreadAttribute PROC_THREAD_ATTRIBUTE_JOB_LIST.

ShellExecuteExW doesn't take a STARTUPINFOEXW structure, and SHELLEXECUTEINFOW doesn't have a similar feature. One can set the SEE_MASK_FLAG_HINST_IS_SITE flag and provide an ICreatingProcess implementation, but ICreateProcessInputs doesn't support setting a job handle.

It would be possible to read SHELLEXECUTEINFOW::hProcess after ShellExecuteExW finishes, and call AssignProcessToJobObject then. However:

  • If the child process already started grandchild processes, then those would not be assigned to the job. That could perhaps be prevented with ICreateProcessInputs::AddCreateFlags(CREATE_SUSPENDED) but that seems likely to cause worse problems, e.g. if ShellExecuteEx waits for the process to do something.
  • If Windows already assigned the child process to a job for application compatibility purposes, then AssignProcessToJobObject would fail. What would .NET do then -- terminate the child process right away?
  • If ShellExecuteExW notified an existing process instead of starting a new one, then no process handle is available. That can't be helped.

A more robust solution might be to start a shim child process with CreateProcess, assign it to a job, and have it call ShellExecuteExW to start the program specified in ProcessStartInfo. That would however require an executable for the shim process, likely also an MSBuild property for configuring whether to publish that shim with the application.

I feel it would be better to make Process.Start throw if ProcessStartInfo requests both UseShellExecute and KillOnParentDeath.

@elijahr2411
Copy link
Author

I've modified the proposal to reflect that ProcessStartInfo should be the class extended.

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 help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants