GeezerVison

PR based on the idea of the PR #13739 but rewritten from scratch on the main branch. The main difference with PR #13739 is that I wrote an unit test ;-) It's non trivial to test this fix manually since UCRT has an undefined behavior: #77046 (comment)

Add test_pipe_spawnl() to test_os.

The test is also run on Unix: IMO it doesn't hurt to run more functional tests on Unix systems ;-)

cc @eryksun @zooba @serhiy-storchaka

Tests / Windows / build and test (x64) (pull_request) Failing after 26m

Oh, test_pipe_spawnl() hangs and is killed after a timeout of 10 minutes :-( It's a debug build, test.pythoninfo says:

build.Py_DEBUG: Yes (sys.gettotalrefcount() present)

The test pass when I built Python locally on my Windows VM.

Oh, test_pipe_spawnl() hangs and is killed after a timeout of 10 minutes

It's probably raising a CRT assertion and blocking. Maybe you can use test.support.SuppressCrashReport in the spawned process?

It's probably raising a CRT assertion and blocking. Maybe you can use test.support.SuppressCrashReport in the spawned process?

Aha. Let me retry using SuppressCrashReport.

@zooba: I updated my PR to pass _O_NOINHERIT flag to _Py_get_osfhandle() callers, instead modifying _Py_get_osfhandle().

I wish Python 3.13 would drop primary support for inheritable file descriptors on Windows, for the sake of reliably correct behavior. os.spawn*() and os.system() could be implemented on Windows via subprocess.Popen, similar to how os.popen() is implemented. We could keep the legacy C functions available as msvcrt.spawnv() and msvcrt.spawnve(), for internal testing and legacy use cases. Just move the source code from "Modules/posixmodule.c" to "PC/msvcrtmodule.c".

The use of inheritable file descriptors is currently problematic in Python on Windows because the C runtime doesn't provide a POSIX fcntl() function with F_GETFD and F_SETFD support. Consequently, os.set_inheritable() isn't supported properly on Windows since it only modifies the underlying OS file handle. Also, os.dup() and os.dup2() are dubiously implemented to return an inheritable file descriptor that's associated with a non-inheritable OS file handle, and there isn't anything we can reasonably do to fix this. It can be a problem for a child process that's spawned by C _wspawn*() if the child happens to have a file opened with the associated handle value at startup (e.g. opened by another startup routine, or duplicated to the process).

os.spawn*() and os.system() could be implemented on Windows via subprocess.Popen, similar to how os.popen() is implemented.

Isn't part of the contract that the FDs have the same fileno in the new process, though? I don't think we can implement that without making our own file descriptor implementation (which I have considered in the past, tbh, but I believe people use them in embeddings and so they really need to be shared with UCRT).

The best we can really do is use platform-specific code in higher level libraries (multiprocessing, etc.) and just say that file descriptors and spawn are inherently not portable to Windows. Meanwhile, we can make our approximation work well enough for sensible/likely scenarios so that Linux code ported to Windows by someone else has a chance of working without needing the original devs to rewrite it all.

Isn't part of the contract that the FDs have the same fileno in the new process, though?

I don't think so. Python should create non inheritable file descriptors (PEP 446), and so they should not be inherited by os.spawnl(). Windows should not behave differently than Unix on that aspect.

Note that _Py_dup() creates non-inheritable descriptor.

Windows C runtime's dup2 creates an inheritable one and clears the FD's NOINHERIT flag, so if you dup2 a non-inheritable FD you'll get an inheritable one, and if you pass inheritable=False then you get an inheritable FD that has a non-inheritable handle which will be invalid in a spawn'd process.

I don't think so.

I was referring to the text that I quoted immediately above my statement. Regardless of where the FD comes from or how it became inheritable, if it is inheritable, then a spawn'd process should have the same FD number used for the same file. Otherwise how do you know how to use the inherited file?

we can make our approximation work well enough for sensible/likely scenarios so that Linux code ported to Windows by someone else has a chance of working without needing the original devs to rewrite it all.

Code written for Linux will use os.set_inheritable() to make a file descriptor inheritable, and then os.spawn*() if it's intended to support platforms that lack os.fork(). That's currently broken on Windows because we cannot make a file-descriptor inheritable, not until the C runtime supports fcntl().

One scenario that works is a two-step process that few would ever think to use. Create a new file descriptor via fd = os.dup(f.fileno()). Follow that with os.dup2(f.fileno(), fd, inheritable=True) to make it inheritable. This could be implemented in a single step if os.dup() supported the inheritable parameter.

Follow that with os.dup2(f.fileno(), fd, inheritable=True) to make it inheritable.

UCRT dup2(fd2, fd) always clears FNOINHERIT flag: the file descriptor is not inheritable, only the handle is inheritable.

Extract of source/lowio/dup2.c:

    // Copy the _osfile information, with the FNOINHERIT bit cleared:
    _osfile(target_fh) = _osfile(source_fh) & ~FNOINHERIT;
    _textmode(target_fh) = _textmode(source_fh);
    _tm_unicode(target_fh) = _tm_unicode(source_fh);

As I wrote previously, on Windows, I suggest to inherit handles and then create (new) file descriptors for them in the child process.

Maybe in Python 3.12, it's possible to inherit some file descriptors on Windows using os.spawnl(), but for me, it's more an accident than a deliberate choice, and the code is not portable.

Another common portable solution is to use a temporary named file rather than a pipe and pass a filename. The disadvantage is that the parent must make sure that the temporary file is deleted.

Python libregrtest has 3 ways to pass JSON from a worker process to its parent:

• Unix: Create a temporary file in the parent and pass a file descriptor to the worker: subprocess.Popen(pass_fds=[fd])
• Windows: Create a temporary file in the parent and pass the handle to the worker: subprocess.STARTUPINFO().lpAttributeList = {"handle_list": [handle]}
• WASI: Use worker process stdout, passing a file descriptor doesn't work on such platform

@zooba @eryksun: We discussed many aspects of inheriting file descriptors on Windows, but are you ok with this specific change?

@serhiy-storchaka: I added comments in the test to clarify that the 2 function calls are expected to fail.

Maybe in Python 3.12, it's possible to inherit some file descriptors on Windows using os.spawnl(), but for me, it's more an accident than a deliberate choice, and the code is not portable.

Well, this change fix a crash affecting Python 3.12 when os.pipe() file descriptors are inherited, whereas they are supposed to be non inheritable: #77046 (comment)

UCRT dup2(fd2, fd) always clears FNOINHERIT flag: the file descriptor is not inheritable, only the handle is inheritable.

Clearing the _NO_INHERIT flag means that the file descriptor is inheritable. I think you misread (or got caught out by the double negative).

I think you misread (or got caught out by the double negative).

That's correct :-D

are you ok with this specific change?

It's already got my approval on it :)

Well, this change fix a crash affecting Python 3.12 when os.pipe() file descriptors are inherited

Really the crash would only be caused by a programming error, where the programmer has incorrectly assumed that the pipe descriptors are inheritable and passed them into a subprocess. Normally, they wouldn't assume it, and wouldn't ever try to access them and so would not crash.

And actually, now that I think through that scenario, we might be breaking people who assume that set_inheritable works properly on those pipes, since it kinda does right now. The FDs are inheritable even though the handles aren't, but set_inheritable fixes the handles and then they should be fine. After this change, set_inheritable now appears to fail, whereas without it, it appears to work. I wonder if that's why it's been left this way in the first place?

It's already got my approval on it :)

Ok, I merged my change. I preferred to make sure that we are on the same page.

After this change, set_inheritable now appears to fail

I'm not sure what you are referring to.

With this change, os.set_inheritable() works as expected on the FDs created by os.pipe():

vstinner@WIN C:\victor\python\main>python
Python 3.13.0a2+ (heads/main:1d75fa43a2, Jan 10 2024, 23:05:33) [MSC v.1938 64 bit (AMD64)] on win32
>>> import os
>>> a,b=os.pipe() 
>>> os.get_inheritable(a)
False

>>> os.set_inheritable(a, True) 
>>> os.get_inheritable(a)       
True

>>> os.set_inheritable(a, False)  
>>> os.get_inheritable(a)        
False

Note: os.pipe() doesn't call set_inheritable() on Windows, only on Unix.

get_inheritable and set_inheritable on Windows refer to the handle, not the file descriptor. They've always worked correctly from that point of view, and always incorrectly with regards to the file descriptor.

os.spawn only shares the file descriptor if the file descriptor is marked as inheritable. It doesn't check the handle.

So previously, you could do this, and a would be shared and usable correctly from the new process (b would fail because the handle is not usable in the new process, even though the file descriptor would be "valid").

a, b = os.pipe()
os.set_inheritable(a, True)
os.spawn(<code that uses 'a'>)

However, because set_inheritable does not change the file descriptor state, and that state is now non-inheritable, the code above will not share a with the new process anymore.

With this change, os.set_inheritable() works as expected on the FDs created by os.pipe()

As I discussed above, os.set_inheritable() does not work on Windows. If the file descriptor is not inheritable, as none should be by default in Python, then trying to make it inheritable via os.set_inheritable() makes the OS handle inheritable, but it does not make the file descriptor itself inheritable. If the file descriptor is inheritable (e.g. as implemented by os.dup2 or msvcrt.open_osfhandle), then trying to make it not inheritable via os.set_inheritable() creates a broken state in which the file descriptor is still inheritable but the OS handle is not. That can be a real problem in some scenarios. Also, os.get_inheritable() reports the wrong information because it only checks the OS handle.

Until UCRT implements POSIX fcntl(), os.set_inheritable() and os.get_inheritable() cannot be implemented on Windows. As I described above, one has to create a new file descriptor with the two-step procedure of os.dup() followed by os.dup2(). That could be simplified to a single step if os.dup() supported the inheritable parameter, or if os.dup() always returned an inheritable file descriptor on Windows. As is, os.dup() returns a file descriptor that's in a broken state in which the file descriptor is inheritable but the OS handle is not.

It's all such a mess that I go back to my original suggestion. Remove support for inheritable file descriptors from the primary os API in Python. Implement os.spawn*() via subprocess.Popen, and wipe our hands clean of this mess.

trying to make it not inheritable via os.set_inheritable() creates a broken state in which the file descriptor is still inheritable but the OS handle is not. That can be a real problem in some scenarios. Also, os.get_inheritable() reports the wrong information because it only checks the OS handle

I'm not so concerned about this scenario, because it requires someone to explicitly make it not inheritable and then explicitly try to inherit it in a child process. That's obviously a coding mistake, and the bad behaviour probably isn't much different than what it would be if we correctly handled things (and likely it's better because there's no risk of the inherited-corrupt file descriptor being replaced by an unrelated one in the new process).

Remove support for inheritable file descriptors from the primary os API in Python. Implement os.spawn*() via subprocess.Popen, and wipe our hands clean of this mess.

I don't entirely disagree, except it's definitely an incompatible change, and so we can't just do it. We probably need to get away from file descriptors entirely for this to be anywhere near reliable, and return the HANDLE value from fileno. I'm not opposed to that, I actually started proposing it at one point, but it's a PEP level change.

Let's end the discussion on this PR. Move back to the issue if needed.