[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Alexey Izbyshev]

Alexey Izbyshev  added the comment:

Great approach :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[STINNER Victor]

STINNER Victor  added the comment:

I wrote PR 23711 (for bpo-32381) which removes half of the problem: it removes 
_Py_fopen() :-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Steve Dower]

Steve Dower  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Alexey Izbyshev]

Alexey Izbyshev  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[STINNER Victor]

STINNER Victor  added the comment:

> 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Eryk Sun]

Eryk Sun  added the comment:

> 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

--
nosy: +eryksun

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Steve Dower]

Steve Dower  added the comment:

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".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Alexey Izbyshev]

Alexey Izbyshev  added the comment:

> 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.)?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[STINNER Victor]

STINNER Victor  added the comment:

> Understanding that seems to be required to deal with issue 32381.

I wrote PR 23642 to fix bpo-32381.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[STINNER Victor]

STINNER Victor  added the comment:

> Could somebody share the current intended status/role of these helpers?

To implement PEP 446: create non-inheritable file descriptors.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42569] Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit hooks

[Alexey Izbyshev]

New submission from Alexey Izbyshev :

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

--
components: Interpreter Core
messages: 382499
nosy: izbyshev, steve.dower, vstinner
priority: normal
severity: normal
status: open
title: Callers of _Py_fopen/_Py_wfopen may be broken after addition of audit 
hooks
type: behavior
versions: Python 3.10, Python 3.8, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com