Skip to content

Add SafeFileHandle.IsInheritable() method#126027

Closed
Copilot wants to merge 6 commits intocopilot/add-standard-input-output-propertiesfrom
copilot/extend-safefilehandle-isinheritable-method
Closed

Add SafeFileHandle.IsInheritable() method#126027
Copilot wants to merge 6 commits intocopilot/add-standard-input-output-propertiesfrom
copilot/extend-safefilehandle-isinheritable-method

Conversation

Copy link
Contributor

Copilot AI commented Mar 24, 2026

Description

Adds SafeFileHandle.IsInheritable() — a new public API that queries the OS to determine whether a file handle is inheritable by child processes.

namespace Microsoft.Win32.SafeHandles;

public sealed partial class SafeFileHandle
{
    public bool IsInheritable();
}

API surface & implementation

  • Reference assembly: Added IsInheritable() to System.Runtime.cs
  • Shared: IsInheritable() in SafeFileHandle.cs validates handle state (ObjectDisposedException.ThrowIf(IsClosed, this)) and delegates to platform-specific IsInheritableCore() which throws for invalid handles
  • Windows: Calls GetHandleInformation and checks HANDLE_FLAG_INHERIT. Added GetHandleInformation P/Invoke to Interop.HandleInformation.cs and linked it in System.Private.CoreLib.Shared.projitems
  • Unix: Calls Fcntl.GetFD (existing SystemNative_FcntlGetFD interop) and checks for absence of FD_CLOEXEC

Codebase usage

  • Updated Process.Windows.cs to use IsInheritable() to skip unnecessary DuplicateAsInheritable calls when handles are already inheritable — resolves an existing TODO comment in that code
  • Updated MemoryMappedFilesTestsBase.Windows.cs to use IsInheritable() in AssertInheritability instead of a local GetHandleInformation P/Invoke

Cleanup

  • Removed unused GetHandleInformation P/Invoke from System.Threading.Overlapped/tests/DllImport.cs
  • Removed unused GetHandleInformation P/Invoke and HANDLE_FLAG_INHERIT constant from System.IO.MemoryMappedFiles/tests/MemoryMappedFilesTestsBase.Windows.cs

Tests

  • Added 5 test methods (7 cases) to OpenHandle.cs:
    • FileShare.None vs FileShare.Inheritable on file handles
    • Same via IntPtr-constructed handles (exercises OS query path)
    • Inheritable pipe via AnonymousPipeServerStream(HandleInheritability.Inheritable)
    • Non-inheritable pipe via SafeFileHandle.CreateAnonymousPipe
    • ObjectDisposedException on disposed handle

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 3 commits March 24, 2026 11:27
…ons and tests

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/4aeb3bf0-77f5-4c75-bfee-36c6937c87fc
@dotnet-policy-service
Copy link
Contributor

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

Copy link
Member

@adamsitnik adamsitnik left a comment

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

…ve unrelated files

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e58ef036-ebe5-4cf6-abab-bfff8da0550b
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please address my feedback

All feedback addressed in 3df4f95: handled IsInvalid, used target-typed new in tests, and removed unrelated 125615.* files.

Copilot AI requested a review from adamsitnik March 24, 2026 13:50
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@copilot address my feedback

…MemoryMappedFiles tests, let IsInheritableCore throw for invalid handles

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c8e2a415-f92e-486e-a525-6231303ef0f8
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot address my feedback

All feedback addressed in 963a15e.

@adamsitnik
Copy link
Member

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review — PR #126027

Holistic Assessment

Motivation: The PR adds a SafeFileHandle.IsInheritable() API that queries the OS for handle inheritability, resolves an existing TODO in Process.Windows.cs, and cleans up duplicate P/Invoke declarations in test code. The motivation is clear and well-justified.

Approach: The implementation follows established patterns — platform-specific Core() methods, shared validation, ObjectDisposedException.ThrowIf, and reusing existing interop (Fcntl.GetFD, GetHandleInformation). The approach is consistent with sibling APIs like IsAsync and Type.

Summary: ✅ LGTM. The change is well-structured, follows codebase conventions, and includes comprehensive tests. No blocking issues found. Minor observations below.


Detailed Findings

✅ API Design — Consistent with established patterns

  • IsInheritable() follows the same structure as the Type property: ObjectDisposedException.ThrowIf(IsClosed, this) then delegate to *Core().
  • The reviewer's direction to let IsInheritableCore() throw for invalid handles (rather than returning false) is the correct design — it's consistent with how GetHandleInformation / fcntl naturally report errors for invalid handles, and avoids silently masking bad state.
  • XML doc comments are complete with platform-specific remarks.

✅ Ref Assembly — Correct placement

  • IsInheritable() is placed alphabetically between IsAsync and Type in System.Runtime.cs. Uses throw null convention. No using directives.

✅ P/Invoke — Correct marshalling

  • GetHandleInformation uses [return: MarshalAs(UnmanagedType.Bool)] (4-byte BOOL) which is the correct convention per dotnet/runtime standards. Takes SafeHandle (not IntPtr) for automatic ref-counting.

✅ Unix Implementation — Follows existing patterns

  • Uses Interop.Sys.Fcntl.GetFD(this) which is the same interop used by SafeSocketHandle.Unix.cs line 216 for the same purpose.
  • FD_CLOEXEC = 1 matches POSIX and is used consistently across the codebase.
  • Error handling with Interop.GetExceptionForIoErrno is the standard Unix error-reporting pattern.

✅ Process.Windows.cs — Correct refactoring

  • DuplicateAsInheritableIfNeeded correctly checks IsInvalid || IsInheritable() before duplicating. The null-coalescing (inheritableStdinHandle ?? stdinHandle).DangerousGetHandle() correctly falls back to the original handle when no duplication was needed.
  • The finally block properly disposes only duplicated handles (?.Dispose()), not the originals.
  • Comments moved from the caller to DuplicateAsInheritableIfNeeded explaining the INVALID_HANDLE_VALUE scenario.

✅ Test Quality — Comprehensive coverage

  • Tests cover both polarities (inheritable and non-inheritable) for file handles, IntPtr-constructed handles, and pipes.
  • Tests use [Theory] with [InlineData] per conventions.
  • Browser platform is correctly skipped for pipe tests.
  • ObjectDisposedException test verifies error handling.

✅ Cleanup — Dead code removal verified

  • GetHandleInformation P/Invoke in DllImport.cs (Overlapped tests) was confirmed unused — no callers found outside the declaration.
  • GetHandleInformation and HANDLE_FLAG_INHERIT in MemoryMappedFilesTestsBase.Windows.cs were replaced by SafeFileHandle.IsInheritable() in AssertInheritability. The Unix variant is a no-op, so the Windows-only assertion via SafeFileHandle is correct.

💡 Minor Observation — MemoryMappedFiles test creates a SafeFileHandle for a non-file handle

In AssertInheritability, a SafeFileHandle is created from a SafeMemoryMappedFileHandle's raw handle. This works because IsInheritable() delegates to GetHandleInformation which operates on any kernel object handle, not just file handles. Since this was specifically requested by the reviewer and the semantics are correct, this is fine.

💡 Minor Observation — projitems ordering

The Interop.HandleInformation.cs entry is added between Interop.PipeOptions.cs and Interop.CreatePipe_SafeFileHandle.cs. Alphabetically, HandleInformation should come before PipeOptions. This is a very minor nit — the existing file doesn't appear to follow strict alphabetical ordering in this section, so this is consistent with the surrounding entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants