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.

diff --git a/Doc/library/os.rst b/Doc/library/os.rst
index c64f9321143731666b2918f4d9a40c74fb4d310e..df136da02cb1fa97ab9a39422a4fa04336ada9e1 100644 (file)
--- 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 a140ae0d29eae12b04f9251d0850f97a8c6adee7..6658a61ea2a74a53430024bf756694cce5ebc489 100644 (file)
--- 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 (file)
index 0000000..117236b
--- /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 <path-like object>` and
+:class:`bytes` objects on Windows.

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 3e03039f15b1c7e34dafe63a5379b87a1de296e5..7f13735eadc8bd5b3e3df8ddf18f1a319f3b7b5a 100644 (file)
--- 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