-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix thread-safety issue in DriveInfo.GetDrives() on macOS by replacing getmntinfo with getfsstat #122637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ad of getmntinfo Replace getmntinfo with getfsstat on macOS/BSD to fix AccessViolationException. getmntinfo uses internal static buffers which are not thread-safe, causing corruption when called concurrently from multiple threads. getfsstat requires user-allocated buffers, making it thread-safe. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
In C, malloc returns void* which implicitly converts to any pointer type, so no explicit cast is needed. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this 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 a critical thread-safety issue in DriveInfo.GetDrives() on macOS/BSD systems that causes random AccessViolationException crashes when called concurrently. The root cause is that getmntinfo() returns pointers to internal static buffers that get corrupted under concurrent access.
Key changes:
- Replace thread-unsafe
getmntinfo()with thread-safegetfsstat()in the HAVE_MNTINFO code path - Implement explicit buffer allocation/deallocation for mount point data
- Switch from
MNT_WAITtoMNT_NOWAITto avoid blocking on slow filesystems
Instead of using a fixed +4 extra buffer slots, implement a retry loop that reallocates and retries if the number of mounts increases between the count query and the data fetch. This handles the edge case where many mounts are added concurrently. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
stephentoub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a special early return case for count==0, just break out of the loop and let the post-loop code handle it. The for loop will iterate 0 times and free(mounts) is safe for NULL pointers. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Build breaks |
|
@copilot Please address the build breaks and my feedback above |
- Add explicit cast for malloc to fix C++ build error - Allocate capacity = count + 1 to detect when more mounts are added - Change condition to count < capacity (since getfsstat returns filled count) - Use (size_t) cast consistently for buffer size parameter Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Fixed in 980c3c7. Added explicit struct casts for the malloc calls to fix the C++ build error. |
- Include pal_safecrt.h for multiply_s - Use multiply_s to compute buffer size with overflow check - Return error on integer overflow - Reuse bufferSize variable for getfsstat call Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
- Add check that bufferSize fits in INT_MAX before using - Cast bufferSize to int when calling getfsstat to fix build break Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Description
DriveInfo.GetDrives()causes randomAccessViolationExceptionon macOS when called concurrently from multiple threads. The root cause isgetmntinfo()returning pointers to internal static buffers that get corrupted under concurrent access.Changes:
getmntinfo()withgetfsstat()inpal_mount.cgetfsstat()uses caller-allocated buffers, making it thread-safeMNT_NOWAITto avoid blocking on slow filesystemscapacity = count + 1to properly detect when more mounts were added (sincegetfsstatreturns the number of structs filled, not the total count)multiply_sfor integer-overflow safe buffer size computationINT_MAXand cast tointforgetfsstatparameterCustomer Impact
Concurrent calls to
DriveInfo.GetDrives()on macOS can crash the process with an unrecoverableAccessViolationException. This is a fatal error that cannot be caught.Regression
No. This has been broken since the API was introduced—the issue exists in .NET 6, 7, and 8.
Testing
HAVE_MNTINFOpath)HAVE_MNTINFOis definedRisk
Low.
getfsstat()is the underlying syscall thatgetmntinfo()wraps. The change adds explicit buffer management with a retry loop but follows the same data flow. The fix is isolated to theHAVE_MNTINFOcode path (macOS/BSD only).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.