This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: os.set_inheritable() fails for O_PATH file descriptors on Linux
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, cptpcrd, miss-islington, vstinner
Priority: normal Keywords: patch

Created on 2020-12-29 17:18 by cptpcrd, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
set-inheritable-o-path.patch cptpcrd, 2020-12-29 17:18
set-inheritable-test.patch cptpcrd, 2021-01-05 13:37
Pull Requests
URL Status Linked Edit
PR 24172 merged cptpcrd, 2021-01-08 20:53
PR 24269 closed miss-islington, 2021-01-20 14:08
PR 24270 closed miss-islington, 2021-01-20 14:10
PR 24277 merged cptpcrd, 2021-01-20 23:03
PR 24278 merged cptpcrd, 2021-01-20 23:03
Messages (12)
msg384016 - (view) Author: (cptpcrd) * Date: 2020-12-29 17:18
Note: I filed this bug report after seeing https://github.com/rust-lang/rust/pull/62425 and verifying that it was also reproducible on Python. Credit for discovering the underlying issue should go to Aleksa Sarai, and further discussion can be found there.

# Background

Linux has O_PATH file descriptors. These are file descriptors that refer to a specific path, without allowing any other kind of access to the file. They can't be used to read or write data; instead, they're intended to be used for use cases like the *at() functions. In that respect, they have similar semantics to O_SEARCH on other platforms (except that they also work on other file types, not just directories).

More information on O_PATH file descriptors can be found in open(2) (https://www.man7.org/linux/man-pages/man2/open.2.html), or in the Rust PR linked above.

# The problem

As documented in the Rust PR linked above, *no* ioctl() calls will succeed on O_PATH file descriptors (they always fail with EBADF). Since os.set_inheritable() uses ioctl(FIOCLEX)/ioctl(FIONCLEX), it will fail on O_PATH file descriptors.

This is easy to reproduce:

>>> import os
>>> a = os.open("/", os.O_RDONLY)
>>> b = os.open("/", os.O_PATH)
>>> os.set_inheritable(a, True)
>>> os.set_inheritable(b, True)  # Should succeed!
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 9] Bad file descriptor
>>>

I believe this affects all versions of Python going back to version 3.4 (where os.set_inheritable()/os.get_inheritable() were introduced).

# Possible fixes

I see two potential paths for fixing this:

1. Don't use ioctl(FIOCLEX) at all on Linux.

This is what Rust did. However, based on bpo-22258 I'm guessing there would be opposition to implementing this strategy in Python, on the grounds that the fcntl() route takes an extra syscall (which is fair).

2. On Linux, fall back on fcntl() if ioctl(FIOCLEX) fails with EBADF.

This could be a very simple patch to Python/fileutils.c. I've attached a basic version of said patch (not sure if it matches standard coding conventions).

Downsides: This would add 2 extra syscalls for O_PATH file descriptors, and 1 extra syscall for actual cases of invalid file descriptors (i.e. EBADF). However, I believe these are edge cases that shouldn't come up frequently.
msg384029 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2020-12-29 21:30
Doing two syscalls does not seem so bad.

Linux may allow FIOCLEX on O_PATH in the future.
msg384035 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-30 00:54
> 2. On Linux, fall back on fcntl() if ioctl(FIOCLEX) fails with EBADF.

I like this approach!
msg384044 - (view) Author: (cptpcrd) * Date: 2020-12-30 02:12
> I like this approach!

Should I put together unit tests to go with the patch? Maybe `test_os.FDInheritanceTests.test_set_inheritable_o_path()`?
msg384396 - (view) Author: (cptpcrd) * Date: 2021-01-05 13:37
I've put together some tests (patch attached). Should I PR this to python/cpython?
msg385339 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-20 14:06
New changeset 7dc71c425cf6aa6a4070a418dce5d95ca435c79f by cptpcrd in branch 'master':
bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172)
https://github.com/python/cpython/commit/7dc71c425cf6aa6a4070a418dce5d95ca435c79f
msg385340 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-20 14:13
Thanks cptpcrd for your bug report and your great fix! I like your tests ;-)
msg385405 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-21 10:47
New changeset 844ec0ba6606b60a59b7da82c54c1e646424259c by cptpcrd in branch '3.8':
bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) (GH-24277)
https://github.com/python/cpython/commit/844ec0ba6606b60a59b7da82c54c1e646424259c
msg385406 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-21 10:47
New changeset 6893523bed4ad701838715380659c245eec71d48 by cptpcrd in branch '3.9':
bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) (GH-24278)
https://github.com/python/cpython/commit/6893523bed4ad701838715380659c245eec71d48
msg385407 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-21 10:48
Thanks cptpcrd for the bug report, the fix and for the two manual backports ;-)

Note: Python 3.6 and 3.7 no longer accept bugfixes.
https://devguide.python.org/#status-of-python-branches
msg385414 - (view) Author: (cptpcrd) * Date: 2021-01-21 13:04
No problem! I've noticed at least one other (relatively minor) issue in `os`, so I may be submitting further bug reports.

I haven't been keeping close track of 3.6/3.7's status, so I added them in without thinking it. Thanks for the reminder.
msg385462 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-01-22 00:58
> No problem! I've noticed at least one other (relatively minor) issue in `os`, so I may be submitting further bug reports.

Please do. But I suggest to open new issues.

> I haven't been keeping close track of 3.6/3.7's status, so I added them in without thinking it. Thanks for the reminder.

Don't worry (be happy). Everybody is confused by the Versions field.
History
Date User Action Args
2022-04-11 14:59:39adminsetgithub: 86946
2021-01-22 00:58:54vstinnersetmessages: + msg385462
2021-01-21 13:04:17cptpcrdsetmessages: + msg385414
2021-01-21 10:48:47vstinnersetmessages: + msg385407
versions: - Python 3.6, Python 3.7
2021-01-21 10:47:04vstinnersetmessages: + msg385406
2021-01-21 10:47:04vstinnersetmessages: + msg385405
2021-01-20 23:03:22cptpcrdsetpull_requests: + pull_request23101
2021-01-20 23:03:21cptpcrdsetpull_requests: + pull_request23100
2021-01-20 14:13:31vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg385340

stage: patch review -> resolved
2021-01-20 14:10:02miss-islingtonsetpull_requests: + pull_request23093
2021-01-20 14:08:18miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request23092
2021-01-20 14:06:25vstinnersetmessages: + msg385339
2021-01-08 20:53:10cptpcrdsetstage: patch review
pull_requests: + pull_request22999
2021-01-05 13:37:15cptpcrdsetfiles: + set-inheritable-test.patch

messages: + msg384396
2020-12-30 02:12:31cptpcrdsetmessages: + msg384044
2020-12-30 00:54:44vstinnersetmessages: + msg384035
2020-12-29 21:30:18benjamin.petersonsetnosy: + benjamin.peterson

messages: + msg384029
versions: + Python 3.6, Python 3.7
2020-12-29 20:34:08izbyshevsetnosy: + vstinner

components: + Library (Lib)
versions: - Python 3.6, Python 3.7
2020-12-29 17:18:38cptpcrdcreate