[Python-checkins] bpo-29734: nt._getfinalpathname handle leak (GH-740)

[Miss Islington (bot)]

https://github.com/python/cpython/commit/63a69ef4a2390cea3e102498ac7eeeb5546e82b6
commit: 63a69ef4a2390cea3e102498ac7eeeb5546e82b6
branch: 3.7
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: GitHub <noreply at github.com>
date: 2019-02-02T13:29:07-08:00
summary:

bpo-29734: nt._getfinalpathname handle leak (GH-740)


Make sure that failure paths call CloseHandle outside of the function that failed
(cherry picked from commit b82bfac4369c0429e562a834b3752e66c4821eab)

Co-authored-by: Mark Becwar <mark at thebecwar.com>

files:
A Misc/NEWS.d/next/Windows/2019-01-12-16-52-38.bpo-29734.6_OJwI.rst
M Lib/test/test_os.py
M Modules/posixmodule.c

diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index fe7261dd76e9..a50eb7e296e6 100644
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -2278,6 +2278,62 @@ def test_unlink_removes_junction(self):
         os.unlink(self.junction)
         self.assertFalse(os.path.exists(self.junction))
 
+ at unittest.skipUnless(sys.platform == "win32", "Win32 specific tests")
+class Win32NtTests(unittest.TestCase):
+    def setUp(self):
+        from test import support
+        self.nt = support.import_module('nt')
+        pass
+
+    def tearDown(self):
+        pass
+
+    def test_getfinalpathname_handles(self):
+        try:
+            import ctypes, ctypes.wintypes
+        except ImportError:
+            raise unittest.SkipTest('ctypes module is required for this test')
+
+        kernel = ctypes.WinDLL('Kernel32.dll', use_last_error=True)
+        kernel.GetCurrentProcess.restype = ctypes.wintypes.HANDLE
+
+        kernel.GetProcessHandleCount.restype = ctypes.wintypes.BOOL
+        kernel.GetProcessHandleCount.argtypes = (ctypes.wintypes.HANDLE,
+                                                 ctypes.wintypes.LPDWORD)
+
+        # This is a pseudo-handle that doesn't need to be closed
+        hproc = kernel.GetCurrentProcess()
+
+        handle_count = ctypes.wintypes.DWORD()
+        ok = kernel.GetProcessHandleCount(hproc, ctypes.byref(handle_count))
+        self.assertEqual(1, ok)
+
+        before_count = handle_count.value
+
+        # The first two test the error path, __file__ tests the success path
+        filenames = [ r'\\?\C:',
+                      r'\\?\NUL',
+                      r'\\?\CONIN',
+                      __file__ ]
+
+        for i in range(10):
+            for name in filenames:
+                try:
+                    tmp = self.nt._getfinalpathname(name)
+                except:
+                    # Failure is expected
+                    pass
+                try:
+                    tmp = os.stat(name)
+                except:
+                    pass
+
+        ok = kernel.GetProcessHandleCount(hproc, ctypes.byref(handle_count))
+        self.assertEqual(1, ok)
+
+        handle_delta = handle_count.value - before_count
+
+        self.assertEqual(0, handle_delta)
 
 @support.skip_unless_symlink
 class NonLocalSymlinkTests(unittest.TestCase):
diff --git a/Misc/NEWS.d/next/Windows/2019-01-12-16-52-38.bpo-29734.6_OJwI.rst b/Misc/NEWS.d/next/Windows/2019-01-12-16-52-38.bpo-29734.6_OJwI.rst
new file mode 100644
index 000000000000..c89a509a0e8f
--- /dev/null
+++ b/Misc/NEWS.d/next/Windows/2019-01-12-16-52-38.bpo-29734.6_OJwI.rst
@@ -0,0 +1 @@
+Fix handle leaks in os.stat on Windows.
\ No newline at end of file
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index f58572b2ceb4..f76bd57fe41b 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -1616,11 +1616,6 @@ get_target_path(HANDLE hdl, wchar_t **target_path)
         return FALSE;
     }
 
-    if(!CloseHandle(hdl)) {
-        PyMem_RawFree(buf);
-        return FALSE;
-    }
-
     buf[result_length] = 0;
 
     *target_path = buf;
@@ -1678,9 +1673,10 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result,
             return -1;
         }
         if (info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
-            if (!win32_get_reparse_tag(hFile, &reparse_tag))
+            if (!win32_get_reparse_tag(hFile, &reparse_tag)) {
+                CloseHandle(hFile);
                 return -1;
-
+            }
             /* Close the outer open file handle now that we're about to
                reopen it with different flags. */
             if (!CloseHandle(hFile))
@@ -1697,8 +1693,14 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result,
                 if (hFile2 == INVALID_HANDLE_VALUE)
                     return -1;
 
-                if (!get_target_path(hFile2, &target_path))
+                if (!get_target_path(hFile2, &target_path)) {
+                    CloseHandle(hFile2);
                     return -1;
+                }
+
+                if (!CloseHandle(hFile2)) {
+                    return -1;
+                }
 
                 code = win32_xstat_impl(target_path, result, FALSE);
                 PyMem_RawFree(target_path);