-
Notifications
You must be signed in to change notification settings - Fork 752
Fix off-by-one error in P/Invoke buffer handling for Windows volume enumeration #2227
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
Conversation
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
src/TraceEvent/TraceEvent.csproj
Outdated
| <Compile Remove="Ctf/CtfTracing.Tests/**/*.*" /> | ||
| <Compile Remove="TraceEvent.Tests/**/*.*" /> | ||
| <Compile Remove="Samples/**/*.*" /> | ||
| <EmbeddedResource Remove="**/*.resx" /> |
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.
Please remove this line from the change.
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.
Removed the EmbeddedResource Remove line as requested in commit 00e1fd6.
| } | ||
|
|
||
| private void Initialize() | ||
| { |
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.
@copilot, can you please write a unit test for this class? I don't think you'll be able to write something that can validate the output, but you can write something that ensures that this code doesn't throw, and outputs the values found in _deviceNameToVolumeNameMap for a developer to view.
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.
Added comprehensive unit tests for WindowsDeviceToVolumeMap class in commit 00e1fd6. The tests verify that the class doesn't throw during initialization and path conversion, and include a test that outputs all discovered device-to-volume mappings for developer inspection. All tests pass successfully on .NET 8.0.
…WindowsDeviceToVolumeMap Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
|
@AaronRobinsonMSFT, would you mind giving this a quick review? I did some brief local testing and things look OK. |
| // Create a string builder which will act as the buffer used to receive the volume and device names. | ||
| StringBuilder builder = new StringBuilder((int)Interop.MAX_PATH, (int)Interop.MAX_PATH); | ||
| // Create a character buffer which will act as the buffer used to receive the volume and device names. | ||
| char[] buffer = new char[Interop.MAX_PATH]; |
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.
Does this need to ever grow? With the \\?\ prefix, I thought the path length can be upwards of 32k.
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.
I don't believe so. From what I can find, these functions are not subject to the 32K limit and are instead subject to the existing MAX_PATH limitation. In fact, 1024 is likely larger than necessary given the documentation.
Here is what I looked at for MAX_PATH limits: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry
Here is an example of calling these specific APIs: https://learn.microsoft.com/en-us/windows/win32/fileio/displaying-volume-paths
Together, the documentation implies that a MAX_PATH of 260 chars is correct.
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.
Okay.
This PR addresses a sporadic crash with
ArgumentOutOfRangeExceptionthat occurs during Windows volume enumeration when the Windows API returns a string that is 1025 characters long (including null terminator) but the StringBuilder buffer was created with both initial capacity and max capacity of 1024.Problem
The issue manifests as:
This occurs in
WindowsDeviceToVolumeMap.Initialize()when calling Windows APIs (FindFirstVolume,FindNextVolume,QueryDosDevice) that useStringBuilderparameters with fixed capacity constraints.Root Cause
The current code used
StringBuilderwith fixed max capacity (1024):When Windows APIs return strings that include the null terminator, they may require 1025 characters total. The StringBuilder cannot expand beyond its max capacity, causing the exception.
Solution
Replaced
StringBuilderparameters withchar[]arrays and implemented manual buffer management:StringBuildertochar[]parameters with explicitCharSet.Unicodechar[1024]buffer with proper null-termination handlingKey Changes
This approach follows Microsoft's P/Invoke best practices and eliminates the capacity constraint issue entirely while maintaining all existing functionality.
Fixes #2226.
💡 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.