-
-
Notifications
You must be signed in to change notification settings - Fork 36.8k
gh-157899: Add support for fds pointing to symlinks in os.readlink(). #157915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2cbd30e
8bb6f09
8196b59
f96963a
7b566b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1881,6 +1881,77 @@ def test_readlink_dir_fd(self): | |
| self.addCleanup(posix.unlink, fullname) | ||
| self.assertEqual(posix.readlink(name, dir_fd=dir_fd), 'symlink') | ||
|
|
||
|
|
||
| _support_readlink_with_fd = hasattr(os, 'readlink') and ( | ||
| "HAVE_FREADLINK" in posix._have_functions # MacOS | ||
| or ( | ||
| os.readlink in os.supports_dir_fd | ||
| and sys.platform in ["linux", "android"] | ||
| ) | ||
| ) | ||
|
Comment on lines
+1885
to
+1891
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd say add them.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, please also test that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
|
||
| def _open_symlink_as_fd(self, path): | ||
| open_flags = os.O_RDONLY | ||
| if hasattr(os, "O_SYMLINK"): # MacOS | ||
| open_flags |= os.O_SYMLINK | ||
| elif hasattr(os, "O_NOFOLLOW") and hasattr(os, "O_PATH"): # Linux | ||
| open_flags |= os.O_NOFOLLOW | os.O_PATH | ||
| else: | ||
| self.fail("lacking open flags for this test") | ||
| return os.open(path, open_flags) | ||
|
|
||
| @unittest.skipUnless(_support_readlink_with_fd, | ||
| "feature not supported on this platform") | ||
| def test_readlink_with_fd(self): | ||
| with self.prepare() as (dir_fd, name, fullname): | ||
| os.symlink("symlink", fullname) | ||
| self.addCleanup(posix.unlink, fullname) | ||
| fd = self._open_symlink_as_fd(fullname) | ||
| self.addCleanup(os.close, fd) | ||
| self.assertEqual(os.readlink(fd), "symlink") | ||
|
|
||
| @unittest.skipUnless(_support_readlink_with_fd, | ||
| "feature not supported on this platform") | ||
| def test_readlink_with_fd_not_referring_to_symlink_throws(self): | ||
| with self.prepare_file() as (dir_fd, name, fullname): | ||
| fd = os.open(fullname, os.O_RDONLY) | ||
| self.addCleanup(os.close, fd) | ||
| # on Linux/Android, readlinkat("", fd, ...) fails with ENOENT, which | ||
| # Python translates to a FileNotFoundError, a subclass of OSError. | ||
| # On MacOS, freadlink(fd, ...) fails with EINVAL, which gets raised | ||
| # as a OSError. | ||
| # So catching OSError here covers both cases. | ||
| with self.assertRaises(OSError): | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My AI was obsessing a bit about this: Depending on circumstances (how you create the FD and the OS) it raises a bare I agree that consistency would be nice, but I thought tinkering with the error handling is probably worse than this little inconsistency.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put this comment in the test code?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a few comment lines into the test. |
||
| os.readlink(fd) | ||
|
|
||
| @unittest.skipUnless(_support_readlink_with_fd, | ||
| "feature not supported on this platform") | ||
| def test_readlink_with_fd_throws_if_both_fd_and_dir_fd_are_given(self): | ||
| with self.prepare() as (dir_fd, name, fullname): | ||
| os.symlink("symlink", fullname) | ||
| self.addCleanup(posix.unlink, fullname) | ||
| fd = self._open_symlink_as_fd(fullname) | ||
| self.addCleanup(os.close, fd) | ||
| with self.assertRaises(ValueError): | ||
| os.readlink(fd, dir_fd=dir_fd) | ||
|
|
||
| @unittest.skipIf(_support_readlink_with_fd, | ||
| "feature is supported on this platform") | ||
| def test_readlink_with_fd_throws_not_implemented_error(self): | ||
| # on unsupported platforms, we may not even be able to get a | ||
| # file descriptor for a symlink, so use a fd for an ordinary file | ||
| os_helper.create_empty_file(os_helper.TESTFN) | ||
| self.addCleanup(os_helper.unlink, os_helper.TESTFN) | ||
| fd = os.open(os_helper.TESTFN, os.O_RDONLY) | ||
| self.addCleanup(os.close, fd) | ||
| with self.assertRaises(NotImplementedError): | ||
| os.readlink(fd) | ||
|
|
||
| @unittest.skipUnless(hasattr(os, "supports_fd") and hasattr(os, "readlink"), | ||
| "feature not supported on this platform") | ||
| def test_readlink_is_in_supports_fd_on_supported_platforms(self): | ||
| self.assertEqual(os.readlink in os.supports_fd, self._support_readlink_with_fd) | ||
|
|
||
| @unittest.skipUnless(os.rename in os.supports_dir_fd, "test needs dir_fd support in os.rename()") | ||
| def test_rename_dir_fd(self): | ||
| with self.prepare_file() as (dir_fd, name, fullname), \ | ||
|
|
@@ -2620,6 +2691,17 @@ def test_readlink(self): | |
| with self.assertRaisesRegex(NotImplementedError, "dir_fd unavailable"): | ||
| os.readlink("path", dir_fd=0) | ||
|
|
||
| def test_freadlink(self): | ||
| self._verify_available("HAVE_FREADLINK") | ||
| if self.mac_ver >= (13, 0): | ||
| self.assertIn("HAVE_FREADLINK", posix._have_functions) | ||
|
|
||
| else: | ||
| self.assertNotIn("HAVE_FREADLINK", posix._have_functions) | ||
|
|
||
| with self.assertRaisesRegex(NotImplementedError, "readlink cannot read file descriptors on this platform"): | ||
| os.readlink(0) | ||
|
|
||
| def test_symlink(self): | ||
| self._verify_available("HAVE_SYMLINKAT") | ||
| if self.mac_ver >= (10, 10): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| :func:`os.readlink` now accepts a file descriptor referring to a | ||
| symlink on Linux, Android and MacOS. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -505,6 +505,7 @@ static const unsigned int _Py_STATX_KNOWN = (STATX_BASIC_STATS | STATX_BTIME | |
| # define HAVE_UNLINKAT_RUNTIME __builtin_available(macOS 10.10, iOS 8.0, *) | ||
| # define HAVE_OPENAT_RUNTIME __builtin_available(macOS 10.10, iOS 8.0, *) | ||
| # define HAVE_READLINKAT_RUNTIME __builtin_available(macOS 10.10, iOS 8.0, *) | ||
| # define HAVE_FREADLINK_RUNTIME __builtin_available(macOS 13.0, *) | ||
| # define HAVE_SYMLINKAT_RUNTIME __builtin_available(macOS 10.10, iOS 8.0, *) | ||
| # define HAVE_FUTIMENS_RUNTIME __builtin_available(macOS 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) | ||
| # define HAVE_UTIMENSAT_RUNTIME __builtin_available(macOS 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) | ||
|
|
@@ -571,6 +572,10 @@ static const unsigned int _Py_STATX_KNOWN = (STATX_BASIC_STATS | STATX_BTIME | |
| # define HAVE_READLINKAT_RUNTIME (readlinkat != NULL) | ||
| # endif | ||
|
|
||
| # ifdef _Py_HAVE_FREADLINK | ||
| # define HAVE_FREADLINK_RUNTIME (freadlink != NULL) | ||
| # endif | ||
|
|
||
| # ifdef HAVE_SYMLINKAT | ||
| # define HAVE_SYMLINKAT_RUNTIME (symlinkat != NULL) | ||
| # endif | ||
|
|
@@ -10986,11 +10991,19 @@ os_unshare_impl(PyObject *module, int flags) | |
| #endif | ||
|
|
||
|
|
||
|
|
||
| #if defined(HAVE_READLINK) || defined(MS_WINDOWS) | ||
|
|
||
| #if (defined(__linux__) || defined(__ANDROID__)) && defined(O_PATH) | ||
| // readlinkat(fd, "", ...) reads the symlink that fd refers to. | ||
| // supported since Linux 2.6.39 (same version that O_PATH was introduced). | ||
| #define _Py_READLINKAT_SUPPORTS_EMPTY_PATH | ||
| #endif | ||
|
|
||
| /*[clinic input] | ||
| os.readlink | ||
|
|
||
| path: path_t | ||
| path: path_t(allow_fd=True) | ||
| * | ||
| dir_fd: dir_fd(requires='readlinkat') = None | ||
|
|
||
|
|
@@ -11002,45 +11015,90 @@ that directory. | |
|
|
||
| dir_fd may not be implemented on your platform. If it is unavailable, | ||
| using it will raise a NotImplementedError. | ||
|
|
||
| On Linux, Android and MacOS, path may be a file descriptor referring to | ||
| a symlink. If it is, dir_fd must be None, and the return value will be a | ||
| bytes object. (File descriptors for symlinks can be obtained with | ||
|
|
||
| os.open(..., os.O_RDONLY | os.O_PATH | os.O_NOFOLLOW) | ||
|
|
||
| on Linux and Android, and | ||
|
|
||
| os.open(..., os.O_RDONLY | os.O_SYMLINK) | ||
|
|
||
| on MacOS.) | ||
| [clinic start generated code]*/ | ||
|
|
||
| static PyObject * | ||
| os_readlink_impl(PyObject *module, path_t *path, int dir_fd) | ||
| /*[clinic end generated code: output=d21b732a2e814030 input=03d10130870dbca8]*/ | ||
| /*[clinic end generated code: output=d21b732a2e814030 input=30272a2c5fba427c]*/ | ||
| { | ||
| #if defined(HAVE_READLINK) | ||
| char buffer[MAXPATHLEN+1]; | ||
| ssize_t length; | ||
| #ifdef HAVE_READLINKAT | ||
| int readlinkat_unavailable = 0; | ||
| #endif | ||
|
|
||
| Py_BEGIN_ALLOW_THREADS | ||
| if (path_and_dir_fd_invalid("readlink", path, dir_fd)) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message here isn't super descriptive - but I figured it was easier to just re-use the existing function. Let me know if you want a change. |
||
| return NULL; | ||
| } | ||
|
|
||
| if (path->is_fd) { | ||
| #if defined(_Py_HAVE_FREADLINK) | ||
| if (HAVE_FREADLINK_RUNTIME) { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| length = freadlink(path->fd, buffer, MAXPATHLEN); | ||
| Py_END_ALLOW_THREADS | ||
| } else { | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "readlink cannot read file descriptors on this platform, " | ||
| "freadlink() is unavailable"); | ||
| return NULL; | ||
| } | ||
| #elif defined(HAVE_READLINKAT) && defined(_Py_READLINKAT_SUPPORTS_EMPTY_PATH) | ||
| // linux/android: | ||
| // readlinkat(fd, "", ...) reads the link that fd refers to. | ||
| if (HAVE_READLINKAT_RUNTIME) { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| length = readlinkat(path->fd, "", buffer, MAXPATHLEN); | ||
| Py_END_ALLOW_THREADS | ||
| } else { | ||
| // this should be unreachable: | ||
| // HAVE_READLINKAT_RUNTIME is always 1 on Linux/Android. | ||
| // Leaving it here as a safeguard. | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "readlink cannot read file descriptors on this platform, " | ||
| "readlinkat() is unavailable"); | ||
| return NULL; | ||
| } | ||
| #else | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "readlink cannot read file descriptors on this platform"); | ||
| return NULL; | ||
|
Comment on lines
+11073
to
+11075
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the reason for having this twice (it's also on line 11113).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one below is in the windows branch. This one is inside the |
||
| #endif | ||
| } else | ||
| #ifdef HAVE_READLINKAT | ||
| if (dir_fd != DEFAULT_DIR_FD) { | ||
| if (HAVE_READLINKAT_RUNTIME) { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| length = readlinkat(dir_fd, path->narrow, buffer, MAXPATHLEN); | ||
| Py_END_ALLOW_THREADS | ||
| } else { | ||
| readlinkat_unavailable = 1; | ||
| argument_unavailable_error(NULL, "dir_fd"); | ||
| return NULL; | ||
| } | ||
| } else | ||
| #endif | ||
| { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| length = readlink(path->narrow, buffer, MAXPATHLEN); | ||
| Py_END_ALLOW_THREADS | ||
|
|
||
| #ifdef HAVE_READLINKAT | ||
| if (readlinkat_unavailable) { | ||
| argument_unavailable_error(NULL, "dir_fd"); | ||
| return NULL; | ||
| Py_END_ALLOW_THREADS | ||
| } | ||
| #endif | ||
|
|
||
| if (length < 0) { | ||
| return path_error(path); | ||
| } | ||
| buffer[length] = '\0'; | ||
|
|
||
| if (PyUnicode_Check(path->object)) | ||
| if (path->is_fd || PyUnicode_Check(path->object)) | ||
| return PyUnicode_DecodeFSDefaultAndSize(buffer, length); | ||
| else | ||
| return PyBytes_FromStringAndSize(buffer, length); | ||
|
|
@@ -11052,6 +11110,12 @@ os_readlink_impl(PyObject *module, path_t *path, int dir_fd) | |
| _Py_REPARSE_DATA_BUFFER *rdb = (_Py_REPARSE_DATA_BUFFER *)target_buffer; | ||
| PyObject *result = NULL; | ||
|
|
||
| if (path->is_fd) { | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "readlink cannot read file descriptors on this platform"); | ||
| return NULL; | ||
| } | ||
|
|
||
| /* First get a handle to the reparse point */ | ||
| Py_BEGIN_ALLOW_THREADS | ||
| reparse_point_handle = CreateFileW( | ||
|
|
@@ -18881,6 +18945,10 @@ PROBE(probe_openat, HAVE_OPENAT_RUNTIME) | |
| PROBE(probe_readlinkat, HAVE_READLINKAT_RUNTIME) | ||
| #endif | ||
|
|
||
| #ifdef _Py_HAVE_FREADLINK | ||
| PROBE(probe_freadlink, HAVE_FREADLINK_RUNTIME) | ||
| #endif | ||
|
|
||
| #ifdef HAVE_SYMLINKAT | ||
| PROBE(probe_symlinkat, HAVE_SYMLINKAT_RUNTIME) | ||
| #endif | ||
|
|
@@ -18948,6 +19016,10 @@ static const struct have_function { | |
| { "HAVE_FPATHCONF", NULL }, | ||
| #endif | ||
|
|
||
| #ifdef _Py_HAVE_FREADLINK | ||
| { "HAVE_FREADLINK", probe_freadlink }, | ||
| #endif | ||
|
|
||
| #ifdef HAVE_FSTATAT | ||
| { "HAVE_FSTATAT", probe_fstatat }, | ||
| #endif | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add Android to the other docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.