classification
Title: os.stat handle leak
Type: behavior Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, eryksun, miss-islington, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch, patch

Created on 2017-03-06 11:26 by eryksun, last changed 2019-04-22 15:46 by berker.peksag. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 740 merged mark.becwar, 2017-03-20 15:36
PR 11740 merged miss-islington, 2019-02-02 21:08
PR 11740 merged miss-islington, 2019-02-02 21:08
PR 12908 merged berker.peksag, 2019-04-22 15:17
Messages (7)
msg289095 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-03-06 11:26
The implementation of nt._getfinalpathname leaks a File handle if calling GetFinalPathNameByHandle fails. The latter function is practically guaranteed to fail when resolving the path for a non-file-system device. It also fails when VOLUME_NAME_DOS is requested for a volume GUID path that isn't currently mounted as either a DOS drive letter or an NTFS junction. In this case requesting VOLUME_NAME_GUID should work.

For example, when I try calling _getfinalpathname to resolve the device paths \\?\MAILSLOT, \\?\PIPE, \\?\UNC, \\?\C:, \\?\PhysicalDrive0, \\?\NUL, \\?\CONIN$, and \\?\COM1, I get the following list of leaked handles:

      0x168 File                  \Device\Mailslot
      0x16c File                  \Device\NamedPipe
      0x178 File                  \Device\Mup
      0x17c File                  \Device\HarddiskVolume2
      0x180 File                  \Device\Harddisk0\DR0
      0x18c File                  \Device\Null
      0x194 File                  \Device\ConDrv
      0x198 File                  \Device\Serial0

(The above is from a context manager that checks for leaked handles using ctypes to call the PssCaptureSnapshot API, which was introduced in Windows 8.1. I think Process Snapshotting is the only Windows API that uses the kernel's ability to fork a clone of a process.) 

The reason that GetFinalPathNameByHandle fails in these cases is that the information classes it queries are typically only serviced by file systems. Other I/O devices (e.g. disk and volume devices) will fail these I/O requests. It happens that GetFinalPathNameByHandle starts with an NtQueryObject request that succeeds in these cases (it's the source of the above native NT device names), but it doesn't stop there. It continues requesting information from the device and the mount-point manager until it either has everything or a request fails.

Also, in os__getfinalpathname_impl, I notice that it's switching from VOLUME_NAME_NT in the first call that's used to get the buffer size to VOLUME_NAME_DOS in the second call. It should use VOLUME_NAME_DOS in both cases, or better yet, add a keyword-only argument to select a different volume-name style (i.e. None, DOS, GUID, or NT).
msg333511 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-01-12 05:49
The os__getfinalpathname_impl handle leak and VOLUME_NAME_NT problems were fixed in another issue. However, PR 740 also includes fixes for handle leaks in os.stat, specifically in win32_xstat_impl and get_target_path.
msg334767 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-02-02 20:51
Steve, can you review and merge PR 740? Mark updated it to fix the handle leaks in win32_xstat_impl as mentioned in my previous message. They're unlikely but still should be fixed. In particular, iIt's unlikely that win32_get_reparse_tag (i.e. FSCTL_GET_REPARSE_POINT) will fail, but not quite as unlikely that get_target_path (i.e. GetFinalPathNameByHandleW) will fail (e.g. it will fail if the target is on an ImDisk ramdisk).
msg334768 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 21:07
We certainly shouldn't be calling CloseHandle on a parameter like this anyway, so the change looks good to me.

However, I notice that we don't necessarily preserve GetLastError() throughout here, so perhaps we ought to consider changing the return value interpretation at some point? (e.g. CloseHandle() may change it, and so we lose the actual error) That can be a future task though.
msg334769 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 21:08
New changeset b82bfac4369c0429e562a834b3752e66c4821eab by Steve Dower (Mark Becwar) in branch 'master':
bpo-29734: nt._getfinalpathname handle leak (GH-740)
https://github.com/python/cpython/commit/b82bfac4369c0429e562a834b3752e66c4821eab
msg334770 - (view) Author: miss-islington (miss-islington) Date: 2019-02-02 21:29
New changeset 63a69ef4a2390cea3e102498ac7eeeb5546e82b6 by Miss Islington (bot) in branch '3.7':
bpo-29734: nt._getfinalpathname handle leak (GH-740)
https://github.com/python/cpython/commit/63a69ef4a2390cea3e102498ac7eeeb5546e82b6
msg340658 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-04-22 15:46
New changeset 6ef726af3ec106013c7c4261ddb306854f2b1778 by Berker Peksag in branch 'master':
bpo-29734: Cleanup test_getfinalpathname_handles test (GH-12908)
https://github.com/python/cpython/commit/6ef726af3ec106013c7c4261ddb306854f2b1778
History
Date User Action Args
2019-04-22 15:46:50berker.peksagsetnosy: + berker.peksag
messages: + msg340658
2019-04-22 15:17:36berker.peksagsetpull_requests: + pull_request12835
2019-02-02 21:29:10miss-islingtonsetnosy: + miss-islington
messages: + msg334770
2019-02-02 21:17:48steve.dowersetkeywords: patch, patch
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-02 21:08:37miss-islingtonsetkeywords: + patch
pull_requests: + pull_request11653
2019-02-02 21:08:35miss-islingtonsetkeywords: + patch
pull_requests: + pull_request11652
2019-02-02 21:08:25steve.dowersetmessages: + msg334769
2019-02-02 21:07:58steve.dowersetmessages: + msg334768
2019-02-02 20:51:47eryksunsetmessages: + msg334767
2019-01-12 05:49:53eryksunsettitle: nt._getfinalpathname handle leak -> os.stat handle leak
stage: patch review
messages: + msg333511
versions: + Python 3.8, - Python 3.5, Python 3.6
2017-03-20 15:36:17mark.becwarsetpull_requests: + pull_request653
2017-03-18 11:41:18mark.becwarsetpull_requests: - pull_request628
2017-03-18 02:17:50mark.becwarsetpull_requests: + pull_request628
2017-03-06 11:26:57eryksuncreate