Code Quality: Replace storage helper P/Invokes - #18936
Conversation
Signed-off-by: Tera <62196528+0x5bfa@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6f11a71f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| short subsNameLength = MemoryMarshal.Read<short>(buffer.AsSpan(10)); | ||
| short printNameOffset = MemoryMarshal.Read<short>(buffer.AsSpan(12)); | ||
| short printNameLength = MemoryMarshal.Read<short>(buffer.AsSpan(14)); | ||
| string pathBuffer = Encoding.Unicode.GetString(buffer, 16, (int)bytesReturned - 16); |
There was a problem hiding this comment.
For every symbolic link, SYMBOLIC_LINK_REPARSE_BUFFER.PathBuffer begins at byte 20 because the four-byte Flags field occupies bytes 16–19, while the name offsets are relative to PathBuffer. Decoding from byte 16 therefore prepends the flags to the substitute name and starts the print name two UTF-16 characters early; the resulting target commonly contains NUL characters and can make Path.GetFullPath throw or otherwise prevent navigation to the link target. Decode from byte 20 (and adjust the returned-length calculation), or retain the prior two-character offset compensation.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| FSCTL_GET_REPARSE_POINT | ||
| IO_REPARSE_TAG_SYMLINK |
There was a problem hiding this comment.
All seven text files modified by this commit contain only LF endings, including these newly added declarations, despite the repository requirement that changed text files use CRLF. Convert the affected files to CRLF before merging so subsequent Windows-side edits do not produce line-ending churn.
AGENTS.md reference: AGENTS.md:L6-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The updated Win32 helper logic introduces correctness and stability regressions (notably symlink reparse buffer parsing and uncaught exceptions in string reads) that can break core navigation/enumeration flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR replaces several storage-related Win32 helper P/Invokes with CsWin32-provided Windows.Win32.PInvoke calls in the Files.App WinUI codebase, aiming to simplify interop usage and improve maintainability while keeping existing storage behaviors.
Changes:
- Switched folder-size and other filesystem enumeration code to use CsWin32
FindFirstFileEx/FindNextFilewithFindCloseSafeHandle. - Updated storage helpers and Win32 helper methods to use CsWin32 APIs for attributes, file handles, and reparse point/ADS handling.
- Extended CsWin32 generation inputs to include
FSCTL_GET_REPARSE_POINT.
File summaries
| File | Description |
|---|---|
| src/Files.App/ViewModels/Properties/Items/FolderProperties.cs | Updates comment references while keeping folder-size behavior aligned with the new Win32 API usage. |
| src/Files.App/ViewModels/Properties/Items/BaseProperties.cs | Migrates folder-size enumeration to CsWin32 FindFirstFileEx/FindNextFile and safe handle disposal. |
| src/Files.App/Utils/Storage/Helpers/StorageHelpers.cs | Replaces attribute probing with CsWin32 GetFileAttributes and updates directory checks. |
| src/Files.App/Helpers/Win32/Win32Helper.Storage.cs | Replaces multiple storage helper interop calls with CsWin32 equivalents (CreateFile, attributes, streams, reparse points). |
| src/Files.App/Helpers/Navigation/NavigationHelpers.cs | Updates symlink tag comparison to use CsWin32 constant. |
| src/Files.App.CsWin32/NativeMethods.txt | Adds FSCTL_GET_REPARSE_POINT to CsWin32 generation list. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static SafeFileHandle OpenFileForRead(string? filePath, bool readWrite = false, uint flags = 0) | ||
| { | ||
| return new SafeFileHandle(Win32PInvoke.CreateFileFromApp(filePath, | ||
| (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | (uint)(readWrite ? FILE_ACCESS_RIGHTS.FILE_GENERIC_WRITE : 0u), (uint)(Win32PInvoke.FILE_SHARE_READ | (readWrite ? 0 : Win32PInvoke.FILE_SHARE_WRITE)), IntPtr.Zero, Win32PInvoke.OPEN_EXISTING, (uint)Win32PInvoke.File_Attributes.BackupSemantics | flags, IntPtr.Zero), true); | ||
| return PInvoke.CreateFile(filePath!, | ||
| (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ | (uint)(readWrite ? FILE_ACCESS_RIGHTS.FILE_GENERIC_WRITE : 0u), FILE_SHARE_MODE.FILE_SHARE_READ | (readWrite ? 0 : FILE_SHARE_MODE.FILE_SHARE_WRITE), null, FILE_CREATION_DISPOSITION.OPEN_EXISTING, FILE_FLAGS_AND_ATTRIBUTES.FILE_FLAG_BACKUP_SEMANTICS | (FILE_FLAGS_AND_ATTRIBUTES)flags, null); | ||
| } |
| public static string? ReadStringFromFile(string filePath) | ||
| { | ||
| IntPtr hFile = Win32PInvoke.CreateFileFromApp(filePath, | ||
| (uint)FILE_ACCESS_RIGHTS.FILE_GENERIC_READ, | ||
| Win32PInvoke.FILE_SHARE_READ, | ||
| IntPtr.Zero, | ||
| Win32PInvoke.OPEN_EXISTING, | ||
| (uint)Win32PInvoke.File_Attributes.BackupSemantics, | ||
| IntPtr.Zero); | ||
|
|
||
| if (hFile.ToInt64() == -1) | ||
| { | ||
| using SafeFileHandle hFile = OpenFileForRead(filePath); | ||
| if (hFile.IsInvalid) | ||
| return null; | ||
| } | ||
|
|
||
| const int BUFFER_LENGTH = 4096; | ||
| byte[] buffer = new byte[BUFFER_LENGTH]; | ||
| int dwBytesRead; | ||
| string szRead = string.Empty; | ||
|
|
||
| unsafe | ||
| { | ||
| using (MemoryStream ms = new MemoryStream()) | ||
| using (StreamReader reader = new StreamReader(ms, true)) | ||
| { | ||
| while (true) | ||
| { | ||
| fixed (byte* pBuffer = buffer) | ||
| { | ||
| if (Win32PInvoke.ReadFile(hFile, pBuffer, BUFFER_LENGTH - 1, &dwBytesRead, IntPtr.Zero) && dwBytesRead > 0) | ||
| { | ||
| ms.Write(buffer, 0, dwBytesRead); | ||
| } | ||
| else | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| ms.Position = 0; | ||
| szRead = reader.ReadToEnd(); | ||
| } | ||
| } | ||
|
|
||
| Win32PInvoke.CloseHandle(hFile); | ||
|
|
||
| return szRead; | ||
| using FileStream stream = new(hFile, FileAccess.Read); | ||
| using StreamReader reader = new(stream, detectEncodingFromByteOrderMarks: true); | ||
| return reader.ReadToEnd(); |
| uint reparseTag = MemoryMarshal.Read<uint>(buffer); | ||
| short subsNameOffset = MemoryMarshal.Read<short>(buffer.AsSpan(8)); | ||
| short subsNameLength = MemoryMarshal.Read<short>(buffer.AsSpan(10)); | ||
| short printNameOffset = MemoryMarshal.Read<short>(buffer.AsSpan(12)); | ||
| short printNameLength = MemoryMarshal.Read<short>(buffer.AsSpan(14)); | ||
| string pathBuffer = Encoding.Unicode.GetString(buffer, 16, (int)bytesReturned - 16); | ||
| var subsString = pathBuffer.Substring(subsNameOffset / 2, subsNameLength / 2); | ||
| var printString = pathBuffer.Substring(printNameOffset / 2, printNameLength / 2); |
Resolved / Related Issues
Steps used to test these changes