Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById - #126705

Merged
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api
Jun 9, 2026
Merged

Implement SafeProcessHandle.Open/TryOpen and Process.TryGetProcessById#126705
adamsitnik merged 29 commits into
mainfrom
copilot/add-safeproceshandle-open-api

Conversation

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the SafeProcessHandle.Open(int processId) and SafeProcessHandle.TryOpen(int processId, out SafeProcessHandle? handle) APIs for opening an existing process by ID, and adds Process.TryGetProcessById.

Also incorporates follow-up fixes for Unix/Windows error handling, API consistency, and API documentation clarity (especially the differences between SafeProcessHandle.[Try]Open and Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById).

APIs approved in #123380 (comment) and #101582 (comment).

Changes

Native layer (pal_process.c, pal_process_wasi.c, pal_process.h, entrypoints.c):

  • Removed SystemNative_OpenProcess and related native/export plumbing
  • Removed now-unneeded #include <sys/syscall.h> from the HAVE_CLOSE_RANGE branch
  • Simplified ForkAndExecOnPDeathSigThread call to return directly

Managed layer:

  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.Kill(processId, 0) directly
  • SafeProcessHandle.Unix.cs: TryOpenCore uses Interop.Sys.GetLastErrorInfo() (compare via .Error, propagate .RawErrno) so Unix error reporting maps correctly
  • SafeProcessHandle.Unix.cs / SafeProcessHandle.Windows.cs: access-denied behavior unified via shared ThrowOpenProcessAccessDeniedException
  • SafeProcessHandle.Windows.cs: OpenCore uses OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE | PROCESS_TERMINATE
  • SafeProcessHandle.cs: Open(int processId) validates arguments, calls TryOpenCore with out int lastError, and throws using returned error
  • TryOpenCore on both platforms returns false only for not-found/invalid-parameter conditions; other failures throw
  • Process.cs: TryGetProcessById validates arguments and aligns with existing GetProcessById semantics by checking ProcessManager.IsProcessRunning and returning a Process instance without eagerly opening a handle
  • Removed managed Unix interop declaration/file for Interop.Sys.OpenProcess
  • Interop.ForkAndExecProcess.cs: inheritedHandles parameter has default null
  • Updated XML docs for SafeProcessHandle.Open / SafeProcessHandle.TryOpen to explicitly document behavioral differences from Process.GetProcessById(...).SafeHandle and Process.TryGetProcessById

Tests:

  • Merged protected-process tests into a single [Theory]
  • Added Windows-specific Open_ExitedProcess_WorksAsLongAsAtLeastOneHandleIsOpen
  • Removed PID-reuse-sensitive flaky tests
  • Restored GetProcessById_KilledProcess_ThrowsArgumentException in ProcessTests.cs
  • Added Assert.NotNull(handle) in ProcessOpenTests before dereferencing nullable handle

Testing

  • Local full process test runs previously validated on both platforms:
    • Windows: System.Diagnostics.Process.Tests total 627, failed 0, skipped 2
    • Linux: System.Diagnostics.Process.Tests total 625, failed 0, skipped 6
  • Post-feedback validation for the latest change set:
    • ./build.sh libs.native -c Debug -os linux -arch x64
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
  • Latest validation after documentation updates:
    • ./build.sh clr+libs+host -rc release -lc release
    • ./dotnet.sh build src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj -c Debug /p:TargetOS=linux /p:TargetArchitecture=x64
    • ./dotnet.sh build /t:test src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj -c Release /p:TargetOS=linux /p:TargetArchitecture=x64 ✅ (Total: 628, Failed: 0, Skipped: 6)

CopilotAIand others added 2 commits April 9, 2026 11:51
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f57caa85-c528-444f-ab43-a628d6ddd488
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI self-assigned this Apr 9, 2026
CopilotAI review requested due to automatic review settings April 9, 2026 12:38
CopilotAI removed the request for review from CopilotApril 9, 2026 12:38

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

…ss with outPidfd parameter
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/664957d4-993f-4f40-aa79-ce45ec55fd11
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 13:26

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot give my feedback a try. Run all process tests and ensure they pass before you push any changes.

Done in e2bc39d. All 405 process tests pass (0 failures, 5 skipped).

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…acOS posix_spawn path
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ed9ebae7-b0ac-4a66-a222-8f7305c55a9b
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotApril 9, 2026 14:36

CopilotAI commented Apr 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

All three review items addressed in a114b09. All 405 process tests pass (0 failures, 5 skipped).

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.c Outdated
…SystemNative_OpenProcess with waitid EPERM fallback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 2, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot address my feedback

Addressed in the latest commit:

  1. SystemNative_OpenProcess (pal_process.c) — when kill(pid, 0) fails with EPERM, now also checks waitid(P_PID, pid, &si, WEXITED | WNOHANG | WNOWAIT). If waitid succeeds (e.g. the child was started as a different user), returns 0; otherwise restores errno = EPERM and returns -1. This keeps GetProcessById/TryGetProcessById consistent with the existing EPERM-as-running treatment on Unix.

  2. Process.TryOpen (Process.Unix.cs) — calls ProcessManager.IsProcessRunning(processId), matching the existing EPERM-treats-as-running behavior of GetProcessById, then creates a SafeProcessHandle via ProcessWaitState.Holder.

  3. Process.TryOpen (Process.Windows.cs) — mirrors the ProcessManager.IsProcessRunning logic using PROCESS_ALL_ACCESS, falls back to GetProcessIds() list check when OpenProcess fails (protected processes, etc.), consistent with GetProcessById.

  4. TryGetProcessById (Process.cs) — now validates arguments itself (ArgumentOutOfRangeException.ThrowIfLessThanOrEqual), calls the platform-specific Process.TryOpen, and handles the processHandle == null fallback path for the Windows list-check case.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment threadsrc/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs Outdated
Comment threadsrc/native/libs/System.Native/pal_process.h Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

@tmds

tmds commented Jun 2, 2026

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

@adamsitnik

Copy link
Copy Markdown
Member

@adamsitnikhttps://github.com/dotnet/runtime/pull/126705/changes#r3328152606 was marked as resolved. Did you see my open question?

I've missed your last question, marked as unresolved and provided my context

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in 000a148.

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

CopilotAI commented Jun 9, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address my feedback

Addressed in e105eec98: I updated the XML docs for SafeProcessHandle.Open and SafeProcessHandle.TryOpen to explicitly call out their behavior differences vs Process.GetProcessById(...).SafeHandle / Process.TryGetProcessById(...).

Note

AI-generated content disclosure: this response was generated with AI assistance.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Process.TryGetProcessById()

5 participants

@tmds@adamsitnik@jkotas