test_os fails on Windows if current directory contains spaces

[serhiy-storchaka]

BPO	31548
Nosy	@pfmoore, @tjguk, @zware, @serhiy-storchaka, @zooba
Files	test_os.log

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-09-21.21:37:06.004>
labels = ['3.7', 'type-bug', 'tests', 'OS-windows']
title = 'test_os fails on Windows if current directory contains spaces'
updated_at = <Date 2017-09-21.21:37:06.004>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2017-09-21.21:37:06.004>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests', 'Windows']
creation = <Date 2017-09-21.21:37:06.004>
creator = 'serhiy.storchaka'
dependencies = []
files = ['47163']
hgrepos = []
issue_num = 31548
keywords = []
message_count = 1.0
messages = ['302728']
nosy_count = 5.0
nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'serhiy.storchaka', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31548'
versions = ['Python 3.7']

• PR: gh-75729: Fix os.spawn tests not handling spaces on Windows #99150

• PR: [3.11] gh-75729: Fix os.spawn tests not handling spaces on Windows (GH-99150) #103366

[serhiy-storchaka]

I just have built Python on Windows first time and ran tests. test_os fails because the current directory path contains spaces.

test_spawnl (test.test_os.SpawnTests) ... C:\Users\Serhiy: can't open file 'Storchaka\py\cpython\PCBuild\win32\python.exe': [Errno 2] No such file or directory
FAIL

[CAM-Gerlach]

Just ran into this issue myself building on Windows—all of the tests under test.test_os.SpawnTests fail due to this issue. I've opened PR #99150 to fix the immediate problem with the tests.

Maybe it would also be a good idea to document this limitation/OS-specific difference, so that users are aware of it? And perhaps even resolve this platform-specific discrepancy for 3.12+ (but I'm assuming that's non-trivial, or it would have been done before)?

[serhiy-storchaka]

It is really very bad API design on Windows. It is a documented quirk, but it is still unsafe and unconvenient.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/spawn-wspawn-functions?view=msvc-170#arguments-for-the-spawned-process

Note

Spaces embedded in strings may cause unexpected behavior; for example, passing _spawn the string "hi there" will result in the new process getting two arguments, "hi" and "there". If the intent was to have the new process open a file named "hi there", the process would fail. You can avoid this by quoting the string: "\"hi there\"".

We now have two options:

1. Document the dangerous behavior on Windows. Every user will need to add a workaround on Windows. And it is more complicated that what the the Microsoft documentation says: for example try to pass a file name "hi\" \"there". So even if the user adds a workaround, in most cases it will be incomplete. That makes the function either non-usable or unsafe on Windows.

2. Try to fix it, so the user will no need to add a workarond, and the code will be the same and safe on all platforms. It is a breaking change, it will break currently working code containing a workaround.

We can also deprecate these functions on Windows or on all platforms. I would say that deprecating broken by design function is a right thing, but now we have correct and predicable implementation on Posix platforms.

I think it requires a wider discussion.

[CAM-Gerlach]

Thanks for the detailed insight!

for example try to pass a file name "hi" "there"

Just to note, " is illegal in filenames on Windows, so for filenames (as in the affected tests), the naive approach to quoting will in fact work in all legal cases. However, this of course isn't, true of arbitrary arguments users may pass here.

I think it requires a wider discussion.

Sure. Would you like me to post a thread about it in the Core Development category on the Python Discourse?

Here's a summary of the possible approaches, as well as current usage:

Preliminaries

Either fixing it or deprecating it will presumably have to be 3.12 only regardless, whereas fixing the docs and tests can and should be backported to bugfix versions, so at minimum, we should:

• Fix the existing tests so they work reliably with the current behavior (in gh-75729: Fix os.spawn tests not handling spaces on Windows #99150)
• Add a warning in the docs explaining the problem and the workaround

Option 1: Fix

To confirm, is the correct argument pre-processing behavior (whether implemented on the user side or in the stdlib itself) basically what the undocumented subprocess.list2cmdline does for subprocess? If so, one approach to fixing this would be to import it from subprocess (inside a function, just like other stuff around it does), and then just add/tweak the Python wrappers for the relevant spawn* functions on Windows (some already have them, some don't) to call list2cmdline on each arg before passing them to the inner functions.

Option 2: Deprecate

Of course, that assumes there's a reason to keep these legacy functions around, instead of just deprecating them in favor of subprocess. If so, IMO, it should be done for the functions as a whole if they are no longer useful, rather than dropping support for them on Windows platforms and keep them on *nix, since it would make existing and future code using them non-portable, and is also potentially more challenging to communicate to users. If they are still useful, it would be both less work on our and users' end to just implement the above fix.

Option 3: Work around

There is a third alternative—document subprocess.list2cmdline publicly and describe in the above docs warning how to use it as a proper workaround when with spawn* (which could even theoretically be backported to bugfix versions, since it is only a documentation change). This would avoid the compatibility issue with existing users while being a fully correct workaround, but still requires every user to use it (correctly) themselves. So, like you, I still prefer the previous two (fix or deprecate).

Survey of current usage

FWIW, there are 438 hits for os\.spawn[lv] on grep.app (matching any of these functions), which is a very small number (there are 30 000 for os.system (:cry:), 13 000 for subprocess.run, 32 000 for subprocess.Popen and 23 000 for suprocess.call, and also less than most of the libraries removed by PEP 594. Checking each of the first 50:

• 17 (34%) are from security tools/demos warning that using this function is unsafe
• 16 (32%) are just vendored copies of Python/its stdlib
• 3 (6%) are platform-specific code/libraries for non-Windows platforms
• 6 (12%) are Scons and vendored copies of it, which uses naive quoting
• 2 (4%) are vendered copies of Pydev, which uses a less naive but still not correct quoting
• 3 (6%) are standalone scripts and packages that are formally retired or very old/unmaintained for 5-10+ years
• 1 (2%) is part of a modern, well used library (opencv), in a utility submodule, and does not use quoting

TL;DR:

• These functions see little use in still-used public Python code, with perhaps ≈20-40 total unique instances across GitHub
• Most genuine unique uses are unquoted, and none actually get the quoting correct
• Over half of non-stdlib hits were actually in security tools/demos warning how it could be exploited

Its also used 2 other places in the stdlib tests, once in a test of os.waitpid and once in a test of tempfile; both already use and require subprocess, so could presumably be migrated to that.

Conclusion

Therefore, it seems reasonable to either fix this issue in 3.12, helping the small number of downstream uses this would adversely affect (those already naively quoting their args) either adapt to the change or migrate to subprocess/etc, or deprecate these functions (and help active users migrate to subprocess).

[serhiy-storchaka]

Would you like me to post a thread about it in the Core Development category on the Python Discourse?

Definitely! Please do this. You wrote a great summary, I would never write something similar.

[CAM-Gerlach]

No, you wrote a great summary, I wrote a long wordy essay, and you're smart to not spend your valuable time writing something similar, as opposed to actually getting things done working on Python :)

I've opened a Discourse thread to discuss this

[CAM-Gerlach]

To note, I've confirmed the os.exec* functions have the exact same issue on Windows; the reason I didn't notice it before was because they didn't have any tests fail, but this is because they don't have any tests where they are supposed to work successfully, I'm guessing because if they did, the test process itself would get replaced with the new process.

These see over 10x or more unique usage, with 2,338 hits on grep.app for os\.exec[lv] plus 123 for from os import exec[vl], and don't have subprocess equivalents. Deprecating these, at least on *nix, doesn't seem like a viable option, so this issue could either be fixed on Windows, or they could be deprecated only on that platform.

[zooba]

I'm guessing because if they did, the test process itself would get replaced with the new process.

I expect any exec tests are going to launch a new process to run the test, because the same would happen on POSIX.

Windows's implementation ought to be the equivalent of p = subprocess.Popen(...); p.wait(); os._exit(p.exitcode), which is probably close enough to keep most users happy.

A big warning in the docs that this is not a portable function, and behaviour could subtly or drastically vary between OSs, seems sufficient for me. I'm pretty sure we already point at subprocess, but I don't see why we need to force casual use of these functions out.

(This isn't the first time I've wanted either a warning or a linter for "potentially non-portable code". Something a dev opts into for any function known to work differently on other platforms when we have a more reliable alternative. This would, of course, report all uses of spawn and exec on any OS, so that devs don't need to run Windows to find out that their code may not work there, which is why a regular warning or deprecation warning is not such a great idea.)