This change should have a unit test, though it is difficult to create one that doesn't cause debug assertion failures in the MSVC runtime library when run.

This change should have a unit test, though it is difficult to create one that doesn't cause debug assertion failures in the MSVC runtime library when run.

Do you mean the dialog box used by the debug CRT for warnings, errors, and failed assertions, such as a failed assertion if an invalid fd is passed to msvcrt.get_osfhandle? That can be suppressed with the context manager support.SuppressCrashReport.

(It also calls SetErrorMode to suppress the WER dialog from an unhandled OS exception -- if the system is configured to show it. An issue should be opened to update this test helper to use msvcrt.SetErrorMode instead of ctypes. We could also add support to msvcrt for GetErrorMode, GetThreadErrorMode and SetThreadErrorMode.)

Thank you for the comment, @eryksun.

Do you mean the dialog box used by the debug CRT for warnings, errors, and failed assertions, such as a failed assertion if an invalid fd is passed to msvcrt.get_osfhandle?

Yes.

An issue should be opened to update this test helper to use msvcrt.SetErrorMode instead of ctypes.

Why exactly? Such a change is not likely to be accepted by CPython unless there is an actual bug to be fixed.

Do you mean the dialog box used by the debug CRT for warnings, errors, and failed assertions,

Yes.

Okay, so a unit test can be added to ensure the fd isn't inherited as long as we suppress these warnings.

Note that fixing this means that os.set_inheritable can no longer enable inheriting these pipe fds via os.spawn*. Maybe a note should be added to explain that there's no simple way in Windows to make the fds inheritable. It's possible, but convoluted. We have to dup the fd; dup2 the result back to the original fd; and then close the duped fd. Once it's made inheritable, there's no way to make the file descriptor properly non-inheritable again. We can only make the underlying OS handle non-inheritable and hope no other file takes that handle value in the child process before the CRT startup code gets a chance to validate the fd. Applications should be encouraged to switch to the subprocess module, which is based on OS handles, instead of file descriptors.

An issue should be opened to update this test helper to use msvcrt.SetErrorMode instead of
ctypes.

Why exactly? Such a change is not likely to be accepted by CPython unless there is an actual
bug to be fixed.

There's no reason to use ctypes if we already have a builtin module that provides the function. Dependence on ctypes in the unit tests should be minimized if possible. One day we might want to make it optional in Windows as well. For example, say if Windows supports an architecture that libffi doesn't support or doesn't support for the required calling convention.

Another issue to be addressed is the Windows implementation of os.dup (_Py_dup in Python/fileutils.c). Someone has a peculiar idea that handles for character (FILE_TYPE_CHAR) files, including console files, cannot be made non-inheritable. Of course they can. Due to this misunderstanding, if we open "NUL", "COM1", "CON", "CONIN$", "CONOUT$", or any other character device, and subsequently dup the fd, the duplicated fd and/or handle is inheritable via os.system, os.spawn*, or subprocess.Popen with close_fds=False.

Maybe the behavior of console applications has confused the author of this code. When a console process is created, if its console is inherited and if its startup info doesn't have the flag STARTF_USESTDHANDLES, then the parent's standard handles (StandardInput, StandardOutput, and StandardError) are duplicated to it, not inherited. The handles don't have to be inheritable, and the resulting handle values may be different in the child. It's okay since standard handles are referenced via the process parameters in the PEB. Subsequently the CRT initializes fds 0-2 from the standard handles. This should agree with fds 0-2 in the parent process (in most cases) because the CRT keeps them in sync with the standard handles as long as a process doesn't directly call WINAPI SetStdHandle.

This change should have a unit test, though it is difficult to create one that doesn't cause debug assertion failures in the MSVC runtime library when run.

IMO the existing test_os.test_pipe() is enough:

    @unittest.skipUnless(hasattr(os, 'pipe'), "need os.pipe()")
    def test_pipe(self):
        rfd, wfd = os.pipe()
        self.addCleanup(os.close, rfd)
        self.addCleanup(os.close, wfd)
        self.assertEqual(os.get_inheritable(rfd), False)
        self.assertEqual(os.get_inheritable(wfd), False)

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Maybe a better approach would replace _open_osfhandle() with _Py_open_osfhandle_noraise(), and modify _Py_open_osfhandle_noraise() so it always sets the _O_NOINHERIT flag.

ISTM this was fixed by #1927.

ISTM this was fixed by #1927.

@erlend-aasland: PR #1927 added _Py_open_osfhandle() helper function to wrap _open_osfhandle() call between _Py_BEGIN_SUPPRESS_IPH and _Py_END_SUPPRESS_IPH, but it doesn't always set the _O_NOINHERIT which is the purpose of this PR.

This change now has merge conflicts and it seems like @ZackerySpytz is no longer active. I wrote a new PR #113817 based on this PR and on the up to date main branch. I close this PR.