[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Changes by Jesús Cea Avión j...@jcea.es: -- nosy: +jcea ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Roundup Robot added the comment: 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 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Roundup Robot added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: Marking as closed/fixed. -- assignee: - larry resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Christian Heimes added the comment: ping... -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Christian Heimes added the comment: 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. :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Christian Heimes added the comment: 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... :) -- nosy: +christian.heimes ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: Patch for 3.3. or trunk? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Christian Heimes added the comment: Preferable both but Coverity Scan uses only trunk. We could commit the patch to trunk first and test if Coverity still detects the leak. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Serhiy Storchaka added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Christian Heimes added the comment: May I suggest a simpler patch that closes the fd sooner? -- Added file: http://bugs.python.org/file31017/listdir_fd.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- Added file: http://bugs.python.org/file31020/larry.listdir.fd.leakage.bug.3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: If someone will give me an LGTM I'll check these in tonight, honest, cross my heart. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- Added file: http://bugs.python.org/file31021/larry.3.3.listdir.fd.leakage.bug.2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- Added file: http://bugs.python.org/file30520/larry.listdir.fd.leakage.bug.2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- Added file: http://bugs.python.org/file30521/larry.3.3.listdir.fd.leakage.bug.1.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Serhiy Storchaka added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Evgeny Kapun added the comment: To make fdopendir fail, just pass any valid FD which points to a non-directory, such as a file or a pipe. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Evgeny Kapun added the comment: Simple test: while True: try: listdir(0) except NotADirectoryError: pass -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Changes by Serhiy Storchaka storch...@gmail.com: -- nosy: +serhiy.storchaka ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
New submission from Evgeny Kapun: 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. -- components: Library (Lib) messages: 188322 nosy: abacabadabacaba priority: normal severity: normal status: open title: os.listdir() leaks FDs if invoked on FD pointing to a non-directory type: resource usage versions: Python 3.3, Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Changes by Antoine Pitrou pit...@free.fr: -- nosy: +larry stage: - needs patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue17899] os.listdir() leaks FDs if invoked on FD pointing to a non-directory
Larry Hastings added the comment: 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. -- keywords: +patch stage: needs patch - patch review Added file: http://bugs.python.org/file30120/larry.listdir.fd.leakage.bug.1.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue17899 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
