| 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) *  |
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: try: listdir(0) except NotADirectoryError: pass |
|
|
| msg190885 - (view) |
Author: Larry Hastings (larry) * |
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) * |
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) * |
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) 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. |
|
|
| msg193518 - (view) |
Author: Christian Heimes (christian.heimes) * |
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) * |
Date: 2013-07-22 09:37 |
| Patch for 3.3. or trunk? |
|
|
| msg193524 - (view) |
Author: Christian Heimes (christian.heimes) * |
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) * |
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 because `close(fd)` can fail and change errno. |
|
|
| msg193557 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-07-22 18:43 |
| May I suggest a simpler patch that closes the fd sooner? |
|
|
| msg193579 - (view) |
Author: Larry Hastings (larry) * |
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) * |
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) * |
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) * |
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) * |
Date: 2013-07-23 08:55 |
| 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. |
|
|
| msg193594 - (view) |
Author: Christian Heimes (christian.heimes) * |
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) * |
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) * |
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) * |
Date: 2013-07-31 19:54 |
| ping... |
|
|
| msg194145 - (view) |
Author: Roundup Robot (python-dev)  |
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(). http://hg.python.org/cpython/rev/e51cbc45f4ca |
|
|
| msg194146 - (view) |
Author: Roundup Robot (python-dev) |
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(). http://hg.python.org/cpython/rev/bf68711bc939 |
|
|
| msg194147 - (view) |
Author: Larry Hastings (larry) * |
Date: 2013-08-02 02:39 |
| Marking as closed/fixed. |
|
|