-
Notifications
You must be signed in to change notification settings - Fork 202
Add expansion of short paths using native Windows call #4068
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
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 encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
I was investigating this issue for the forked version we use and was going to submit a PR, but this was already in progress! For some background about what caused this:
The bug does not seem to affect directories with spaces in the name, only ones without spaces. That's why the unit test for stats:
childStats:
|
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.
LGTM
@LWSimpkins Thanks for the context! It's interesting that I can't reproduce this on my Windows machine. I've created a test account
|
@koesie10 Huh, interesting. I wonder if the version of Windows also matters. Either way, your solution will fix the issue :) |
It seems like in certain cases we cannot resolve Windows short paths and fail with an error message in the CodeQL CLI. As a fallback mechanism, this adds a native call to the
GetLongPathNameW
win32 API function, which will correctly resolve the path if it exists. To reduce the cost of calling this function, we are now also only expanding the short paths once instead of twice per variant analysis run.To call this function, we're using koffi. koffi includes a native Node.js extension for calling into the C functions and bundles this extension for a lot of platforms by default:
This would increase the size of our extension by tens of megabytes (all native extensions together are about 66 MB), so I've worked around this by only bundling Windows x64 by default. The call is only made on Windows, and I expect x64 to be the only common Windows platform. In total, this adds about 2.2MB to the extension.
It's been tested by the user that reported the issue and this seems to have fixed the issue (#4059 (comment)), and I've also tested it on one of my own Windows machines where it resolves the path correctly (although the original workaround also works there, so I've artificially made it use the native method).