Title: os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.3, Python 3.4
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: abacabadabacaba, christian.heimes, jcea, larry, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-05-03 19:16 by abacabadabacaba, last changed 2014-02-17 16:57 by jcea. This issue is now closed.

File name Uploaded Description Edit
larry.listdir.fd.leakage.bug.1.patch larry, 2013-05-04 00:19 First cut at a patch fixing the fd leaked when fdopendir fails in os.listdir. review
larry.listdir.fd.leakage.bug.2.patch larry, 2013-06-10 02:24 review
larry.3.3.listdir.fd.leakage.bug.1.patch larry, 2013-06-10 02:31 review
listdir_fd.patch christian.heimes, 2013-07-22 18:43 review
larry.listdir.fd.leakage.bug.3.patch larry, 2013-07-23 03:55 review
larry.3.3.listdir.fd.leakage.bug.2.patch larry, 2013-07-23 04:09 review
Messages (24)
msg188322 - (view) Author: Evgeny Kapun (abacabadabacaba) Date: 2013-05-03 19:16
When called with a file descriptor as an argument, os.listdir() duplicates it to pass to fdopendir(3). If this call fails, the new file descriptor is not closed, which leads to file descriptor leak.
msg188339 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-05-04 00:19
Patch attached.  I don't know how to make fdopendir fail, so I had to do it by inspection.

While I was in there, I ifdef'd out the variable that should only be used if fdopendir is available.
msg188340 - (view) Author: Evgeny Kapun (abacabadabacaba) Date: 2013-05-04 06:14
To make fdopendir fail, just pass any valid FD which points to a non-directory, such as a file or a pipe.
msg188341 - (view) Author: Evgeny Kapun (abacabadabacaba) Date: 2013-05-04 06:17
Simple test:

while True:
    except NotADirectoryError:
msg190885 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-06-10 02:24
Second rev incorporating a suggestion from the ever-present Serhiy.  Also, for what it's worth, I walked through this with the debugger when using os.listdir(0) and it worked fine.
msg190886 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-06-10 02:31
Here's a patch for 3.3.  There's been enough churn around listdir in trunk that I was gonna have to write the patches separately anyway.
msg190887 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-06-10 05:53
rewinddir() is called only when dirp != NULL && fd > -1. fdopendir() is called when fd != -1. close() is called when dirp == NULL && fd != -1. Therefore rewinddir() and fdopendir() with close() can't be called in the same time. And you can move block

    if (dirp == NULL)

up, just after fdopendir(). In all other branches fd == -1 and close() is not called. You will save #ifdef HAVE_FDOPENDIR/#endif and Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS lines.
msg193518 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-22 08:45
I'd like to have a patch by tonight. This issue is one of two remaining bugs that are considered "high impact" by Coverity Scan. The other one looks like a false positive. I have an interview with Coverity tomorrow about the development style and quality of CPython's source code and how we are using their software. I would like to show off a bit... :)
msg193521 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-22 09:37
Patch for 3.3. or trunk?
msg193524 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-22 10:31
Preferable both but Coverity Scan uses only trunk. We could commit the patch to trunk first and test if Coverity still detects the leak.
msg193550 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-22 17:14
Larry, you forgot to remove "#undef HAVE_FDOPENDIR" at the start of the file. Beside this line the patch LGTM. I withdraw my objections in msg190887 because `close(fd)` can fail and change errno.
msg193557 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-22 18:43
May I suggest a simpler patch that closes the fd sooner?
msg193579 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 03:44
I've been staring at the code.  I just realized: we really should call path_error() as soon as possible once we detect the error--as Christian's patch points out, close() will clear the error.  So instead of playing footsie with assigning (further) to errno, I'm leaving the if (dirp == NULL) code where it is.  It won't happen any sooner execution-wise; moving the close up only makes it slightly(?) easier to read, at the expense of duplicating some code.

Christian, the simplicity of your patch misses one minor point: when FDOPENDIR is not defined, fd is never used, and therefore emits a warning.  I was trying to clean that up at the same time as fixing this bug.
msg193580 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 03:55
Attached is a new patch for trunk, removing the #undef HAVE_FDOPENDIR debug scaffolding, and rearranging the lines of error handling so close doesn't clear the errno before we use it.

By cracky, while most days I do enjoy the exacting pedantry of the Python community, it sure can slow the process down sometimes.
msg193581 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 04:09
And here's an updated patch for 3.3; the only change from the previous 3.3 patch is that I call path_error before calling close.
msg193582 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 04:10
If someone will give me an LGTM I'll check these in tonight, honest, cross my heart.
msg193591 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 08:55
Looks like it's too late for Christian, he's already generated the report with Coverity:

But I'm still interested in putting this to bed.
msg193594 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-23 10:02
That's a preliminary report I made a couple of days ago. I posted it on G+ for some friends, it got re-posted by the official Python account and gone viral on Twitter.

The interview is tonight. Coverity is probably going to create their own report after the interview.

The "save_errno" approach is a common technique and not a hack. On modern systems with threading support the errno variable is a carefully crafted macro that wraps an internal function + thread local storage. Several Python files like signalmodule, faulthandler and more are using save_errno.

Yes, we are pedantic. Sometimes it's not easy to cope with but we are creating a hell of a shiny piece of software. :)
msg193615 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-23 17:16
I swear I've seen a compiler where you couldn't assign to errno.  But now I'm pretty sure it was MSVC, and possibly a version back in the 90s.  So I'm happy to live in a world where errno is an lvalue.
msg193716 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-07-25 20:12
Okay, this bug has dragged on long enough.  Serhiy already said they looked good to him, and I am now declaring that that's good enough for me.

I'll check in my patches Saturday-ish unless someone pipes up.
msg194013 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-31 19:54
msg194145 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-08-02 01:21
New changeset e51cbc45f4ca by Larry Hastings in branch 'default':
Issue #17899: Fix rare file descriptor leak in os.listdir().
msg194146 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-08-02 02:39
New changeset bf68711bc939 by Larry Hastings in branch '3.3':
Issue #17899: Fix rare file descriptor leak in os.listdir().
msg194147 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-08-02 02:39
Marking as closed/fixed.
Date User Action Args
2014-02-17 16:57:32jceasetnosy: + jcea
2013-08-02 02:39:59larrysetstatus: open -> closed
messages: + msg194147

assignee: larry
resolution: fixed
stage: patch review -> resolved
2013-08-02 02:39:13python-devsetmessages: + msg194146
2013-08-02 01:21:50python-devsetnosy: + python-dev
messages: + msg194145
2013-07-31 19:54:50christian.heimessetmessages: + msg194013
2013-07-25 20:12:02larrysetmessages: + msg193716
2013-07-23 17:16:19larrysetmessages: + msg193615
2013-07-23 10:02:02christian.heimessetmessages: + msg193594
2013-07-23 08:55:53larrysetmessages: + msg193591
2013-07-23 04:10:06larrysetmessages: + msg193582
2013-07-23 04:09:36larrysetfiles: + larry.3.3.listdir.fd.leakage.bug.2.patch

messages: + msg193581
2013-07-23 03:56:00larrysetfiles: + larry.listdir.fd.leakage.bug.3.patch

messages: + msg193580
2013-07-23 03:44:22larrysetmessages: + msg193579
2013-07-22 18:43:29christian.heimessetfiles: + listdir_fd.patch

messages: + msg193557
2013-07-22 17:14:09serhiy.storchakasetmessages: + msg193550
2013-07-22 10:31:33christian.heimessetmessages: + msg193524
2013-07-22 09:37:04larrysetmessages: + msg193521
2013-07-22 08:45:46christian.heimessetnosy: + christian.heimes
messages: + msg193518
2013-06-30 17:43:23christian.heimeslinkissue18332 superseder
2013-06-10 05:53:38serhiy.storchakasetmessages: + msg190887
2013-06-10 02:31:42larrysetfiles: + larry.3.3.listdir.fd.leakage.bug.1.patch

messages: + msg190886
2013-06-10 02:24:33larrysetfiles: + larry.listdir.fd.leakage.bug.2.patch

messages: + msg190885
2013-05-04 08:29:33serhiy.storchakasetnosy: + serhiy.storchaka
2013-05-04 06:17:17abacabadabacabasetmessages: + msg188341
2013-05-04 06:14:58abacabadabacabasetmessages: + msg188340
2013-05-04 00:19:24larrysetfiles: + larry.listdir.fd.leakage.bug.1.patch
keywords: + patch
messages: + msg188339

stage: needs patch -> patch review
2013-05-03 23:23:51pitrousetnosy: + larry

stage: needs patch
2013-05-03 19:16:26abacabadabacabacreate