Skip to content

Fix missing error handling in SafeFileHandle.Type on Windows#126618

Merged
adamsitnik merged 4 commits intomainfrom
copilot/add-error-handling-safe-file-handle
Apr 8, 2026
Merged

Fix missing error handling in SafeFileHandle.Type on Windows#126618
adamsitnik merged 4 commits intomainfrom
copilot/add-error-handling-safe-file-handle

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 7, 2026 •
edited
Loading

Description

SafeFileHandle.Type silently returns FileHandleType.Unknown on Windows when called with an invalid handle, instead of throwing an exception. This is because GetFileType returns FILE_TYPE_UNKNOWN (0) for both "type is genuinely unknown" and "an error occurred" — the distinction requires checking GetLastError.

Fix: In GetFileTypeCore(), handle FILE_TYPE_UNKNOWN before the switch expression. Check Marshal.GetLastPInvokeError() and throw a Win32 exception if the error code is non-zero; only return FileHandleType.Unknown when GetLastError is ERROR_SUCCESS.

// Before: always returned Unknown for invalid handles
_ => FileHandleType.Unknown

// After: throws for invalid handles, returns Unknown only when genuinely unknown
if (kernelFileType == FILE_TYPE_UNKNOWN)
{
    int error = Marshal.GetLastPInvokeError();
    if (error != Interop.Errors.ERROR_SUCCESS)
        throw Win32Marshal.GetExceptionForWin32Error(error, Path);
    return FileHandleType.Unknown;
}

Changes

  • SafeFileHandle.Windows.cs — extract FILE_TYPE_UNKNOWN check before switch in GetFileTypeCore(), add GetLastError check to throw on error

Copilot AI requested review from Copilot and removed request for Copilot April 7, 2026 20:44
Copilot AI linked an issue Apr 7, 2026 that may be closed by this pull request
Missing error handling in SafeFileHandle.Type on Windows #126617
Closed
Copilot AI requested review from Copilot and removed request for Copilot April 7, 2026 20:53
Copilot AI changed the title [WIP] Add missing error handling in SafeFileHandle.Type on Windows Fix missing error handling in SafeFileHandle.Type on Windows Apr 7, 2026
Copilot AI requested a review from jkotas April 7, 2026 20:54
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

dotnet-policy-service bot commented Apr 7, 2026

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

adamsitnik
adamsitnik reviewed Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
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 add a test that creates an invalid SafeFileHandle with a positive handle value and ensures that Type throws the same exception on every platform. One idea would be to open a regular file with File.OpenHandle, then create a copy of the handle then close the original handle and then call the Type property for the copy

Copilot AI requested review from Copilot and removed request for Copilot April 7, 2026 22:03
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 7, 2026

@copilot add a test that creates an invalid SafeFileHandle with a positive handle value and ensures that Type throws the same exception on every platform. One idea would be to open a regular file ...

Added GetFileType_InvalidHandle_Throws in GetFileType.cs (commit 275074d): opens a file, wraps the native handle value in a non-owning SafeFileHandle, disposes the original (invalidating the OS handle), and then asserts .Type throws via AssertExtensions.ThrowsAny<IOException, UnauthorizedAccessException> — covering both Windows (IOException from ERROR_INVALID_HANDLE) and Unix (UnauthorizedAccessException from EBADF).

Copilot AI requested a review from adamsitnik April 7, 2026 22:04
Copilot AI requested review from Copilot and removed request for Copilot April 7, 2026 22:31
Copilot AI requested a review from jkotas April 7, 2026 22:32
@jkotas jkotas marked this pull request as ready for review April 7, 2026 22:32
Copilot AI review requested due to automatic review settings April 7, 2026 22:32
Copilot AI requested a review from jkotas April 7, 2026 22:36
Copilot AI reviewed Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes Windows-specific behavior in SafeFileHandle.Type so that invalid handles no longer silently report FileHandleType.Unknown; instead, the implementation checks GetLastError when GetFileType returns FILE_TYPE_UNKNOWN and throws a Win32-based exception when an actual error occurred.

Changes:

  • Add a pre-switch FILE_TYPE_UNKNOWN branch in GetFileTypeCore() to distinguish “unknown type” from “error” via Marshal.GetLastPInvokeError().
  • Throw Win32Marshal.GetExceptionForWin32Error(...) when GetLastError != ERROR_SUCCESS; otherwise return FileHandleType.Unknown.

@adamsitnik
Copy link
Copy Markdown
Member

adamsitnik commented Apr 8, 2026

This is not reliable. The handle could have been reused.

It is complicated to test this reliably for the same reasons discussed in #126318. I do not think we need a test for this.

@jkotas what if we put this test in a dedicated class and annotate it with following attribute that ensures other tests are not running in parallel?

runtime/src/libraries/Common/tests/TestUtilities/System/DisableParallelization.cs

Lines 8 to 11 in 68b538c

// The collection definitions must be in the same assembly as the test that uses them.
// So please use "Compile Include" in the project file to include this class.
[CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)]
public class DisableParallelization { }

Can runtime still open some new handles/descriptors in the meantime?

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 8, 2026 •
edited
Loading

Can runtime still open some new handles/descriptors in the meantime?

Yes, you have GC threads, finalizer thread, tiered compilation thread, debugger thread, diagnostic server thread, and several PAL threads all running in the background doing stuff that you do not have a control over.

adamsitnik
adamsitnik approved these changes Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
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.

LGTM, thank you @jkotas !

@adamsitnik
Copy link
Copy Markdown
Member

adamsitnik commented Apr 8, 2026

/ba-g the failures are unrelated

@adamsitnik adamsitnik merged commit 8843355 into main Apr 8, 2026
154 of 158 checks passed
@adamsitnik adamsitnik deleted the copilot/add-error-handling-safe-file-handle branch April 8, 2026 06:42
@adamsitnik adamsitnik added this to the 11.0.0 milestone Apr 8, 2026
@dotnet-maestro dotnet-maestro bot mentioned this pull request Apr 9, 2026
Merged
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
@jkotas
@adamsitnik @radekdoulik
Fix missing error handling in SafeFileHandle.Type on Windows (dotnet#…
cf65cd9 
…126618)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkotas jkotas jkotas approved these changes

Copilot code review Copilot Copilot left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

@jkotas jkotas

Copilot code review Copilot

Labels

area-System.IO

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

Missing error handling in SafeFileHandle.Type on Windows

4 participants

@adamsitnik @jkotas