From 63a69ef4a2390cea3e102498ac7eeeb5546e82b6 Mon Sep 17 00:00:00 2001 From: "Miss Islington (bot)" <31488909+miss-islington@users.noreply.github.com> Date: Sat, 2 Feb 2019 13:29:07 -0800 Subject: [PATCH] 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 --- Lib/test/test_os.py | 56 +++++++++++++++++++ .../2019-01-12-16-52-38.bpo-29734.6_OJwI.rst | 1 + Modules/posixmodule.c | 18 +++--- 3 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2019-01-12-16-52-38.bpo-29734.6_OJwI.rst diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index fe7261dd76e..a50eb7e296e 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2278,6 +2278,62 @@ class Win32JunctionTests(unittest.TestCase): os.unlink(self.junction) self.assertFalse(os.path.exists(self.junction)) +@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 00000000000..c89a509a0e8 --- /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 f58572b2ceb..f76bd57fe41 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);