os.listdir() leaks FDs if invoked on FD pointing to a non-directory

[abacabadabacaba]

BPO	17899
Nosy	@jcea, @larryhastings, @tiran, @serhiy-storchaka
Files	larry.listdir.fd.leakage.bug.1.patch: First cut at a patch fixing the fd leaked when fdopendir fails in os.listdir. larry.listdir.fd.leakage.bug.2.patch larry.3.3.listdir.fd.leakage.bug.1.patch listdir_fd.patch larry.listdir.fd.leakage.bug.3.patch larry.3.3.listdir.fd.leakage.bug.2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/larryhastings'
closed_at = <Date 2013-08-02.02:39:59.298>
created_at = <Date 2013-05-03.19:16:26.371>
labels = ['library', 'performance']
title = 'os.listdir() leaks FDs if invoked on FD pointing to a non-directory'
updated_at = <Date 2014-02-17.16:57:32.762>
user = 'https://bugs.python.org/abacabadabacaba'

bugs.python.org fields:

activity = <Date 2014-02-17.16:57:32.762>
actor = 'jcea'
assignee = 'larry'
closed = True
closed_date = <Date 2013-08-02.02:39:59.298>
closer = 'larry'
components = ['Library (Lib)']
creation = <Date 2013-05-03.19:16:26.371>
creator = 'abacabadabacaba'
dependencies = []
files = ['30120', '30520', '30521', '31017', '31020', '31021']
hgrepos = []
issue_num = 17899
keywords = ['patch']
message_count = 24.0
messages = ['188322', '188339', '188340', '188341', '190885', '190886', '190887', '193518', '193521', '193524', '193550', '193557', '193579', '193580', '193581', '193582', '193591', '193594', '193615', '193716', '194013', '194145', '194146', '194147']
nosy_count = 6.0
nosy_names = ['jcea', 'larry', 'christian.heimes', 'abacabadabacaba', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue17899'
versions = ['Python 3.3', 'Python 3.4']

[abacabadabacaba]

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.

[larryhastings]

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.

[abacabadabacaba]

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

[abacabadabacaba]

Simple test:

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

[larryhastings]

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.

[larryhastings]

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.

[serhiy-storchaka]

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.

[tiran]

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

[larryhastings]

Patch for 3.3. or trunk?

[tiran]

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

[serhiy-storchaka]

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.

[tiran]

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

[larryhastings]

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.

[larryhastings]

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.

[larryhastings]

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.

[larryhastings]

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

[larryhastings]

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.

[tiran]

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

[larryhastings]

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.

[larryhastings]

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.

[tiran]

ping...

[python-dev]

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

[python-dev]

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

[larryhastings]

Marking as closed/fixed.