From e0b5b2096ead4cc094a129ce7602ac5c0e998086 Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Wed, 15 Aug 2018 13:03:41 +0300 Subject: [PATCH] bpo-34384: Fix os.readlink() on Windows (GH-8740) os.readlink() now accepts path-like and bytes objects on Windows. Previously, support for path-like and bytes objects was only implemented on Unix. This commit also merges Unix and Windows implementations of os.readlink() in one function and adds basic unit tests to increase test coverage of the function. --- Doc/library/os.rst | 4 +- Lib/test/test_os.py | 47 ++++++++++ .../2018-08-12-08-43-21.bpo-34384.yjofCv.rst | 2 + Modules/posixmodule.c | 93 +++++++------------ 4 files changed, 86 insertions(+), 60 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-08-12-08-43-21.bpo-34384.yjofCv.rst diff --git a/Doc/library/os.rst b/Doc/library/os.rst index c64f9321143..df136da02cb 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -2014,8 +2014,10 @@ features: The *dir_fd* argument. .. versionchanged:: 3.6 - Accepts a :term:`path-like object`. + Accepts a :term:`path-like object` on Unix. + .. versionchanged:: 3.8 + Accepts a :term:`path-like object` and a bytes object on Windows. .. function:: remove(path, *, dir_fd=None) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index a140ae0d29e..6658a61ea2a 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2039,6 +2039,53 @@ class Win32ListdirTests(unittest.TestCase): [os.fsencode(path) for path in self.created_paths]) +@unittest.skipUnless(hasattr(os, 'readlink'), 'needs os.readlink()') +class ReadlinkTests(unittest.TestCase): + filelink = 'readlinktest' + filelink_target = os.path.abspath(__file__) + filelinkb = os.fsencode(filelink) + filelinkb_target = os.fsencode(filelink_target) + + def setUp(self): + self.assertTrue(os.path.exists(self.filelink_target)) + self.assertTrue(os.path.exists(self.filelinkb_target)) + self.assertFalse(os.path.exists(self.filelink)) + self.assertFalse(os.path.exists(self.filelinkb)) + + def test_not_symlink(self): + filelink_target = FakePath(self.filelink_target) + self.assertRaises(OSError, os.readlink, self.filelink_target) + self.assertRaises(OSError, os.readlink, filelink_target) + + def test_missing_link(self): + self.assertRaises(FileNotFoundError, os.readlink, 'missing-link') + self.assertRaises(FileNotFoundError, os.readlink, + FakePath('missing-link')) + + @support.skip_unless_symlink + def test_pathlike(self): + os.symlink(self.filelink_target, self.filelink) + self.addCleanup(support.unlink, self.filelink) + filelink = FakePath(self.filelink) + self.assertEqual(os.readlink(filelink), self.filelink_target) + + @support.skip_unless_symlink + def test_pathlike_bytes(self): + os.symlink(self.filelinkb_target, self.filelinkb) + self.addCleanup(support.unlink, self.filelinkb) + path = os.readlink(FakePath(self.filelinkb)) + self.assertEqual(path, self.filelinkb_target) + self.assertIsInstance(path, bytes) + + @support.skip_unless_symlink + def test_bytes(self): + os.symlink(self.filelinkb_target, self.filelinkb) + self.addCleanup(support.unlink, self.filelinkb) + path = os.readlink(self.filelinkb) + self.assertEqual(path, self.filelinkb_target) + self.assertIsInstance(path, bytes) + + @unittest.skipUnless(sys.platform == "win32", "Win32 specific tests") @support.skip_unless_symlink class Win32SymlinkTests(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Library/2018-08-12-08-43-21.bpo-34384.yjofCv.rst b/Misc/NEWS.d/next/Library/2018-08-12-08-43-21.bpo-34384.yjofCv.rst new file mode 100644 index 00000000000..117236b21b0 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-08-12-08-43-21.bpo-34384.yjofCv.rst @@ -0,0 +1,2 @@ +:func:`os.readlink` now accepts :term:`path-like ` and +:class:`bytes` objects on Windows. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 3e03039f15b..7f13735eadc 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7414,18 +7414,24 @@ If dir_fd is not None, it should be a file descriptor open to a directory,\n\ and path should be relative; path will then be relative to that directory.\n\ dir_fd may not be implemented on your platform.\n\ If it is unavailable, using it will raise a NotImplementedError."); -#endif -#ifdef HAVE_READLINK - -/* AC 3.5: merge win32 and not together */ static PyObject * posix_readlink(PyObject *self, PyObject *args, PyObject *kwargs) { path_t path; int dir_fd = DEFAULT_DIR_FD; +#if defined(HAVE_READLINK) char buffer[MAXPATHLEN+1]; ssize_t length; +#elif defined(MS_WINDOWS) + DWORD n_bytes_returned; + DWORD io_result; + HANDLE reparse_point_handle; + + char target_buffer[_Py_MAXIMUM_REPARSE_DATA_BUFFER_SIZE]; + _Py_REPARSE_DATA_BUFFER *rdb = (_Py_REPARSE_DATA_BUFFER *)target_buffer; + const wchar_t *print_name; +#endif PyObject *return_value = NULL; static char *keywords[] = {"path", "dir_fd", NULL}; @@ -7436,6 +7442,7 @@ posix_readlink(PyObject *self, PyObject *args, PyObject *kwargs) READLINKAT_DIR_FD_CONVERTER, &dir_fd)) return NULL; +#if defined(HAVE_READLINK) Py_BEGIN_ALLOW_THREADS #ifdef HAVE_READLINKAT if (dir_fd != DEFAULT_DIR_FD) @@ -7455,45 +7462,11 @@ posix_readlink(PyObject *self, PyObject *args, PyObject *kwargs) return_value = PyUnicode_DecodeFSDefaultAndSize(buffer, length); else return_value = PyBytes_FromStringAndSize(buffer, length); -exit: - path_cleanup(&path); - return return_value; -} - -#endif /* HAVE_READLINK */ - -#if !defined(HAVE_READLINK) && defined(MS_WINDOWS) - -static PyObject * -win_readlink(PyObject *self, PyObject *args, PyObject *kwargs) -{ - const wchar_t *path; - DWORD n_bytes_returned; - DWORD io_result; - PyObject *po, *result; - int dir_fd; - HANDLE reparse_point_handle; - - char target_buffer[_Py_MAXIMUM_REPARSE_DATA_BUFFER_SIZE]; - _Py_REPARSE_DATA_BUFFER *rdb = (_Py_REPARSE_DATA_BUFFER *)target_buffer; - const wchar_t *print_name; - - static char *keywords[] = {"path", "dir_fd", NULL}; - - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "U|$O&:readlink", keywords, - &po, - dir_fd_unavailable, &dir_fd - )) - return NULL; - - path = _PyUnicode_AsUnicode(po); - if (path == NULL) - return NULL; - +#elif defined(MS_WINDOWS) /* First get a handle to the reparse point */ Py_BEGIN_ALLOW_THREADS reparse_point_handle = CreateFileW( - path, + path.wide, 0, 0, 0, @@ -7502,8 +7475,10 @@ win_readlink(PyObject *self, PyObject *args, PyObject *kwargs) 0); Py_END_ALLOW_THREADS - if (reparse_point_handle==INVALID_HANDLE_VALUE) - return win32_error_object("readlink", po); + if (reparse_point_handle == INVALID_HANDLE_VALUE) { + return_value = path_error(&path); + goto exit; + } Py_BEGIN_ALLOW_THREADS /* New call DeviceIoControl to read the reparse point */ @@ -7518,26 +7493,31 @@ win_readlink(PyObject *self, PyObject *args, PyObject *kwargs) CloseHandle(reparse_point_handle); Py_END_ALLOW_THREADS - if (io_result==0) - return win32_error_object("readlink", po); + if (io_result == 0) { + return_value = path_error(&path); + goto exit; + } if (rdb->ReparseTag != IO_REPARSE_TAG_SYMLINK) { PyErr_SetString(PyExc_ValueError, "not a symbolic link"); - return NULL; + goto exit; } print_name = (wchar_t *)((char*)rdb->SymbolicLinkReparseBuffer.PathBuffer + rdb->SymbolicLinkReparseBuffer.PrintNameOffset); - result = PyUnicode_FromWideChar(print_name, - rdb->SymbolicLinkReparseBuffer.PrintNameLength / sizeof(wchar_t)); - return result; + return_value = PyUnicode_FromWideChar(print_name, + rdb->SymbolicLinkReparseBuffer.PrintNameLength / sizeof(wchar_t)); + if (path.narrow) { + Py_SETREF(return_value, PyUnicode_EncodeFSDefault(return_value)); + } +#endif +exit: + path_cleanup(&path); + return return_value; } - -#endif /* !defined(HAVE_READLINK) && defined(MS_WINDOWS) */ - - +#endif /* defined(HAVE_READLINK) || defined(MS_WINDOWS) */ #ifdef HAVE_SYMLINK @@ -12950,16 +12930,11 @@ static PyMethodDef posix_methods[] = { OS_GETPRIORITY_METHODDEF OS_SETPRIORITY_METHODDEF OS_POSIX_SPAWN_METHODDEF -#ifdef HAVE_READLINK +#if defined(HAVE_READLINK) || defined(MS_WINDOWS) {"readlink", (PyCFunction)posix_readlink, METH_VARARGS | METH_KEYWORDS, readlink__doc__}, -#endif /* HAVE_READLINK */ -#if !defined(HAVE_READLINK) && defined(MS_WINDOWS) - {"readlink", (PyCFunction)win_readlink, - METH_VARARGS | METH_KEYWORDS, - readlink__doc__}, -#endif /* !defined(HAVE_READLINK) && defined(MS_WINDOWS) */ +#endif /* defined(HAVE_READLINK) || defined(MS_WINDOWS) */ OS_RENAME_METHODDEF OS_REPLACE_METHODDEF OS_RMDIR_METHODDEF