classification
Title: Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, izbyshev, steve.dower, vstinner
Priority: normal Keywords:

Created on 2020-12-04 16:12 by izbyshev, last changed 2020-12-09 00:22 by izbyshev.

Messages (11)
msg382499 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-12-04 16:12
Before addition of audit hooks in 3.8, _Py_fopen() and _Py_wfopen() were simple wrappers around corresponding C runtime functions. They didn't require GIL, reported errors via errno and could be safely called during early interpreter initialization.

With the addition of PySys_Audit() calls, they can also raise an exception, which makes it unclear how they should be used. At least one caller[1] is confused, so an early-added hook (e.g. from sitecustomize.py) that raises a RuntimeError on at attempt to open the main file causes the following:

$ ./python /home/test/test.py
./python: can't open file '/home/test/test.py': [Errno 22] Invalid argument
Traceback (most recent call last):
  File "/home/test/.local/lib/python3.10/site-packages/sitecustomize.py", line 10, in hook
    raise RuntimeError("XXX")
RuntimeError: XXX

"Invalid argument" is reported by pymain_run_file() due to a bogus errno, and the real problem (exception from the hook) is noticed only later.

Could somebody share the current intended status/role of these helpers? Understanding that seems to be required to deal with issue 32381.

[1] https://github.com/python/cpython/blob/066394018a84/Modules/main.c#L314
msg382507 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-04 16:49
> Could somebody share the current intended status/role of these helpers?

To implement PEP 446: create non-inheritable file descriptors.
msg382508 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-04 16:51
> Understanding that seems to be required to deal with issue 32381.

I wrote PR 23642 to fix bpo-32381.
msg382513 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-12-04 17:13
> To implement PEP 446: create non-inheritable file descriptors.

Yes, I understand that was the original role. But currently there is no easy way to deal with errors from the helpers because of exception vs. errno conundrum. Maybe they should be split to two functions each (one that always reports errors via an exception, and the other is a low-level one)? Or, alternatively, keep only exception-reporting variants but check all callers so that they don't use errno and call them from the right context (with GIL, etc.)?
msg382521 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-12-04 19:53
If the GIL is not held, no exception should be raised, so the fact that you've got one there means that the GIL is held and the main.c-specific error message is the bit that's wrong.

So it should be, "if they fail and you're in a context where exceptions are allowed, raise an exception" (which will chain back to the one raised from an audit hook".
msg382540 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2020-12-04 22:35
> To implement PEP 446: create non-inheritable file descriptors.

Side note. That aspect is still wonky in Windows, for which set_inheritable() cannot be implemented reliably since there's no way to change whether an existing CRT file descriptor is inheritable. The current implementation just puts the combination of CRT fd and OS handle in a bad state (discussed in more detail in bpo-32865, an issue about fixing os.pipe, but related). In Windows, the fopen() and _wfopen() calls here should use the non-standard "N" flag [1] to open a non-inheritable file descriptor and skip calling set_inheritable().

---

[1] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-160
msg382549 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-04 23:48
> In Windows, the fopen() and _wfopen() calls here should use the non-standard "N" flag [1] to open a non-inheritable file descriptor and skip calling set_inheritable().

Interesting. Do you want to propose a PR to enhance the Python implementation?
msg382589 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-12-06 08:15
> So it should be, "if they fail and you're in a context where exceptions are allowed, raise an exception" (which will chain back to the one raised from an audit hook".

What exception should be raised if _Py_fopen() fails (returns NULL)? We can't just check errno anymore because _Py_fopen() might have failed because of the audit hook and thus didn't set errno. So it seems to me that addition of audit hooks broke the implicit contract of _Py_fopen()/_Py_wfopen(), and callers were not fixed.
msg382658 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-12-07 16:49
Good point, we can make it set errno as well. Any generic error is fine (I don't know them off the top of my head), because it will chain up to the one set by the hook.
msg382777 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-08 23:38
I wrote PR 23711 (for bpo-32381) which removes half of the problem: it removes _Py_fopen() :-)
msg382779 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2020-12-09 00:22
Great approach :)
History
Date User Action Args
2020-12-09 00:22:36izbyshevsetmessages: + msg382779
2020-12-08 23:38:21vstinnersetmessages: + msg382777
2020-12-07 16:50:01steve.dowersetmessages: + msg382658
2020-12-06 08:15:29izbyshevsetmessages: + msg382589
2020-12-04 23:48:09vstinnersetmessages: + msg382549
2020-12-04 22:35:14eryksunsetnosy: + eryksun
messages: + msg382540
2020-12-04 19:53:33steve.dowersetmessages: + msg382521
2020-12-04 17:13:47izbyshevsetmessages: + msg382513
2020-12-04 16:51:24vstinnersetmessages: + msg382508
2020-12-04 16:49:26vstinnersetmessages: + msg382507
2020-12-04 16:12:33izbyshevcreate