Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(249019)

#24505: shutil.which wrong result on Windows

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by bobjalex
Modified:
3 years, 7 months ago
Reviewers:
eryksun, toby.tobkin, eryksun+pybugs
CC:
pmoore, haypo, tim.golden, r.david.murray, Zach Ware, eryksun, steve.dower, bobjalex_gmail.com, bar.harel, Toby Tobkin
Visibility:
Public.

Patch Set 1 #

Total comments: 24
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/shutil.py View 1 chunk +111 lines, -39 lines 24 comments Download
Lib/test/test_shutil.py View 2 chunks +71 lines, -1 line 0 comments Download

Messages

Total messages: 7
eryksun
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1139 Lib/shutil.py:1139: if file_ext == '': This isn't how cmd behaves. ...
3 years, 8 months ago #1
Toby Tobkin
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1139 Lib/shutil.py:1139: if file_ext == '': On 2015/12/17 22:57:50, eryksun wrote: ...
3 years, 8 months ago #2
eryksun
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1106 Lib/shutil.py:1106: def _is_windows_nt_internal_command(_cmd): Consider making this a module-level attribute, present ...
3 years, 8 months ago #3
Toby Tobkin
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1106 Lib/shutil.py:1106: def _is_windows_nt_internal_command(_cmd): On 2015/12/22 07:49:19, eryksun wrote: > Consider ...
3 years, 7 months ago #4
Toby Tobkin
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1106 Lib/shutil.py:1106: def _is_windows_nt_internal_command(_cmd): On 2015/12/22 07:49:19, eryksun wrote: > Consider ...
3 years, 7 months ago #5
eryksun+pybugs_gmail.com
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1139 Lib/shutil.py:1139: if file_ext == '': On 2015/12/31 06:41:15, Toby Tobkin ...
3 years, 7 months ago #6
Toby Tobkin
3 years, 7 months ago #7
http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py
File Lib/shutil.py (right):

http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1139
Lib/shutil.py:1139: if file_ext == '':
On 2015/12/31 14:20:42, eryksun wrote:
> On 2015/12/31 06:41:15, Toby Tobkin wrote:
> > 
> > However, it appears that PATHEXT is the feature to 
> > rely on for determining if a file is executable 
> > from the shell's point of view.
> 
> You must be experimenting in PowerShell, which actually does use PATHEXT as
you
> describe. But we're looking to copy cmd's behavior, right?
> 
> Tim Hill's book, which you cited, states that cmd always searches for the
exact
> filename and also with each of the PATHEXT extensions appended. That's
correct.
> However, he's demonstrably wrong in claiming that the PATHEXT extensions are
> only used if the searched command lacks an extension. For example, cmd will
find
> "filename.exe.exe" if you try to run "filename.exe", even if an actual file
> named "filename.exe" exists in a directory that's farther along in PATH.

You are correct; sorry about that. I did test it out and e.g. "filename.exe"
will match "filename.exe.exe". I'll address that in my third patch and attach it
to the original issue.
 
> 
> > The documentation seems to indicate that 
> > AssocQueryString is only needed for situations 
> > of opening "documents" or "data." 
> 
> When cmd finds a match it first tries to run it via CreateProcess, regardless
of
> the file extension. CreateProcess can run 32-bit and 64-bit PE executables,
and
> it can also run .bat/.cmd batch files via the %ComSpec% interpreter (this
> defaults to cmd.exe).
> 
> If CreateProcess fails with either ERROR_BAD_EXE_FORMAT or
> ERROR_ELEVATION_REQUIRED, then cmd tries to run the file via ShellExecuteEx
> instead. We could emulate this by calling subprocess.Popen with
> creationflags=DETACHED_PROCESS|CREATE_SUSPENDED. If the call fails with
> ERROR_ELEVATION_REQUIRED, we're done. If it fails with ERROR_BAD_EXE_FORMAT,
> then we know to try AssocQueryString. 
> 
> subprocess.Popen also gives us a real AccessCheck for free. It fails with
> ERROR_ACCESS_DENIED if the file security doesn't allow or denies execute
access
> for the user. In this case, if we're staying true to cmd, we hault the search
> and return the match even though the user lacks execute access.
> 
> It's ok if CreateProcess doesn't fail. Since the process is suspended, the
main
> (and only) thread never gets to execute any code. It's a zombie that we can
> simply terminate. We know it's a PE executable or batch file, so we're done.
> 
> Otherwise AssocQueryString comes into play. It's the best clue we have as to
> what ShellExecuteEx would actually run if called. cmd calls ShellExecuteEx
with
> lpVerb as NULL to execute the default action for the file. Maybe it's a data
> file that opens in a viewer or editor. Or maybe it's a script that runs as
> '"C:\Program Files\Python35\python.exe" "script.py" %*'. Should we narrow it
> down by checking whether the command-line template (ASSOCSTR_COMMAND) contains
> %2 through %9 or %* for passing commmand-line arguments? The cmd shell doesn't
> make this distinction. It will happily shell-execute coolvideo.mp4 by starting
a
> video player to view it.

How did you learn all this about cmd's behavior? I spent at least 10 hours
researching much of this, writing toy programs in Visual C++, and playing with
cmd/Powershell, and I couldn't come up with this on my own.

In any case, I think this makes enough sense for me to complete the
implementation. Thanks for giving me such a detailed pointer.

http://bugs.python.org/review/24505/diff/16179/Lib/shutil.py#newcode1139
Lib/shutil.py:1139: if file_ext == '':
On 2015/12/31 14:20:42, eryksun wrote:
> It's not unheard of to use ctypes in the standard library. For example, it's
> used in platform.py. It's discouraged because ctypes is optional, but it's
> always available on Windows. 
> 
> However, please avoid using ctypes.windll. It caches WinDLL instances, which
> cache function pointers. The standard library should not be at the mercy of
> whoever has defined prototypes (restype, argtypes, errcheck) for functions.
Use
> kernel32 = WinDLL('kernel32') instead. If you need the Windows LastError
value,
> use WinDLL('kernel32', use_last_error=True). This thread-local value is
accessed
> via the functions ctypes.set_last_error and ctypes.get_last_error. Use
> ctypes.WinError(ctypes.get_last_error()) to get a formatted exception.

Got it. I'll make that change.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+