Conversation
|
This approach seems a little hacky, and surely someone has faced this same concern on calling a subprocess on Windows before. I would love to see a reference to prior art or documented best practice before adopting such a change. |
I found this which I think is the exact same category: Also a related cpython issue: python/cpython#91558 |
In that example (jupyter/nbconvert#2261) the code checks that it's on windows, in the current working directory, and not on the path, and only for Python versions less than 3.12 (it seems in Python 3.12 the Windows specific behavior changed that makes this issue moot(?): https://docs.python.org/3/library/shutil.html#shutil.which) Your solution only checks if it's in the current working directory. I would recommend a |
On Windows shutil.which() and CreateProcess() search the current directory ahead of PATH, so a planted rustc.exe or keyring.exe wins over the real one. Route both lookups through which_outside_cwd(), which drops a match in the cwd, unless the cwd is explicitly on PATH, and returns an absolute path.
08463ca to
1370378
Compare
That is because on nbconvert's case NoDefaultCurrentDirectoryInExePath was added on 3.12 so they need to check it manually for <3.12 but the env variable approach wouldn't work here as os.environ will have the variable inherited to every child process so build backends could be affected. Hence the full check.
Pushed a change that implements the approach, basically consent is implied if cwd is in the PATH, if not, rejection. This is a little bit less strict that what I was going for but it matches nbconvert. Also with my previous stricter version there could be a potential issue such as cd'ing into ~/.cargo/bin, running pip and the real rustc would be rejected. |
On Windows shutil.which() and CreateProcess() search the current directory ahead of PATH, so a planted rustc.exe or keyring.exe wins over the real one.
Route both lookups through which_outside_cwd(), which drops a match in the cwd and returns an absolute path.
What does this PR do?
Fixes: #14294
PR Checklist:
I've used Claude to help me identify the issue and test it on a Windows 11 VM to verify, along with testing the fix. It was also used to review my fixes.