homepage

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.

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.

To make fdopendir fail, just pass any valid FD which points to a non-directory, such as a file or a pipe.

Simple test:
while True:
 try:
 listdir(0)
 except NotADirectoryError:
 pass

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.

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.

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)
 close(fd);
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.

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... :)

Patch for 3.3. or trunk?

Preferable both but Coverity Scan uses only trunk. We could commit the patch to trunk first and test if Coverity still detects the leak.

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.

May I suggest a simpler patch that closes the fd sooner?

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.

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.

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.

If someone will give me an LGTM I'll check these in tonight, honest, cross my heart.

Looks like it's too late for Christian, he's already generated the report with Coverity:
https://docs.google.com/file/d/0B5wQCOK_TiRiMWVqQ0xPaDEzbkU/edit
But I'm still interested in putting this to bed.

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. :)

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.

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.

ping...

New changeset e51cbc45f4ca by Larry Hastings in branch 'default':
Issue #17899: Fix rare file descriptor leak in os.listdir().
http://hg.python.org/cpython/rev/e51cbc45f4ca 

New changeset bf68711bc939 by Larry Hastings in branch '3.3':
Issue #17899: Fix rare file descriptor leak in os.listdir().
http://hg.python.org/cpython/rev/bf68711bc939 

Marking as closed/fixed.