Skip to content

Conversation

StephanTLavavej
Copy link
Member

We have an extremely squirrelly line of code that's memcpying two consecutive DWORDs into the beginning of a buffer. There's no reason for this weirdness - it isn't perf-critical, and the optimizer should understand memcpy. Now, code analysis tools (specifically CodeQL) are noticing that this code is a 🐿️ read overrun. Let's avoid this by splitting it up into two separate reads.

For the destination, _Id points to FILE_ID_INFO. Its FileId is FILE_ID_128, which contains BYTE Identifier[16];.

For the source, _Info is BY_HANDLE_FILE_INFORMATION:

typedef struct _BY_HANDLE_FILE_INFORMATION {
  DWORD    dwFileAttributes;
  FILETIME ftCreationTime;
  FILETIME ftLastAccessTime;
  FILETIME ftLastWriteTime;
  DWORD    dwVolumeSerialNumber;
  DWORD    nFileSizeHigh;
  DWORD    nFileSizeLow;
  DWORD    nNumberOfLinks;
  DWORD    nFileIndexHigh;
  DWORD    nFileIndexLow;
} BY_HANDLE_FILE_INFORMATION, *PBY_HANDLE_FILE_INFORMATION, *LPBY_HANDLE_FILE_INFORMATION;

@StephanTLavavej StephanTLavavej added enhancement Something can be improved filesystem C++17 filesystem labels Sep 4, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner September 4, 2024 05:21
@StephanTLavavej StephanTLavavej self-assigned this Sep 9, 2024
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 479dcb9 into microsoft:main Sep 9, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the the-unbeatable-squirrel-filesystem branch September 9, 2024 19:48
@ariccio
Copy link

ariccio commented Mar 16, 2025

(sorry to resurrect an obscure old issue & pr)

Do you or anybody else remember what codeql query was responsible for detecting this? I'm catching up on all that I've missed in MSVC for the past 5-6 years, and this piqued my interest. I'm super glad to see that tools have kept improving!

I remember back in the day when working on tools to account for disk space usage that some of these APIs made me a bit nervous and static analysis wasn't quite up to it yet.

@StephanTLavavej
Copy link
Member Author

It was a cpp/overflow-buffer warning SM01947 "Call to memory access function may overflow buffer" (filed as internal VSO-2255048).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved filesystem C++17 filesystem
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants