Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
2013/8/27 Antoine Pitrou :
>> On UNIX, the subprocess module closes almost all file descriptors in
>> the child process. This operation requires MAXFD system calls, where
>> MAXFD is the maximum number of file descriptors, even if there are
>> only few open file descriptors. This maximum can be read using:
>> os.sysconf("SC_OPEN_MAX").
>
> If your intent is to remove the closerange() call from subprocess, be
> aware that it may let through some file descriptors opened by
> third-party code (such as C extensions). This may or may not be
> something we want to worry about, but there's still a small potential
> for security regressions.
The PEP doesn't change the default value of the close_fds parameter of
subprocess: file descriptors and handles are still closed in the child
process.
I modified the PEP to explain the link between non-inheritable FDs and
performances:
http://hg.python.org/peps/rev/d88fbf9941fa
If you don't use third party code, or if you control third party code
and you know that these modules only create non-inheritable FDs, it is
now safe (thanks to the PEP 446) to use close_fds=False... which
avoids the cost of closing MAXFD file descriptors explicitly in the
child process.
Victor
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Hi,
I have a small comment to make:
> On UNIX, the subprocess module closes almost all file descriptors in
> the child process. This operation requires MAXFD system calls, where
> MAXFD is the maximum number of file descriptors, even if there are
> only few open file descriptors. This maximum can be read using:
> os.sysconf("SC_OPEN_MAX").
If your intent is to remove the closerange() call from subprocess, be
aware that it may let through some file descriptors opened by
third-party code (such as C extensions). This may or may not be
something we want to worry about, but there's still a small potential
for security regressions.
Regards
Antoine.
___
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Wow, that was quick! I propose that we wait for one more day for any feedback from others in response to this post, and then accept the PEP. On Mon, Aug 26, 2013 at 3:19 PM, Victor Stinner wrote: > 2013/8/26 Guido van Rossum : > > I have reviewed the PEP and I think it is good. Thank you so much for > > pushing this topic and for your very thorough review of all the feedback, > > related issues and so on. It is an exemplary PEP! > > Thanks :-) I updated the PEP: > http://hg.python.org/peps/rev/edd8250f6893 > > > I've made a bunch of small edits (mostly to improve grammar slightly, > hope > > you don't mind) and committed these to the repo. > > Thanks, I'm not a native english speaker, so not problem for such edit. > > > > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode437 > > pep-0446.txt:437: condition. > > As C-F Natali pointed out, this is not actually a problem, because after > > fork() > > only the main thread survives. Maybe just delete this paragraph? > > Ok, I didn't know that only one thread survives to fork(). (I read > Charles' email, but I forgot to update the PEP.) I simply deleted the > paragraph. > > > > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode450 > > pep-0446.txt:450: parameter is a non-empty list of file descriptors. > > Well, it could pass closefrom() the max of the given list and manually > close > > the > > rest. This would be useful if the system max is large but none of the FDs > > given > > in the list is. (This would be more complex code but it would address the > > issue > > for most programs.) > > This was related to the multi-thread issue, which does not exist, so I > also removed this paragraph. > > Using closefrom() to optimize subprocess is unrelated to this PEP. > > (And yes, the maximum file descriptor can be huge!) > > > > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode538 > > pep-0446.txt:538: descriptors). > > I would say it should not be changed because the default is still better. > > :-) > > (The PEP does not propose to change the default value.) > > Under Linux, recent versions of the glibc uses non-inheritable FD for > internal files. Slowly, more and more libraries and programs will do > the same. This PEP is a step in this direction ;-) > > Victor > -- --Guido van Rossum (python.org/~guido) ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
2013/8/26 Guido van Rossum : > I have reviewed the PEP and I think it is good. Thank you so much for > pushing this topic and for your very thorough review of all the feedback, > related issues and so on. It is an exemplary PEP! Thanks :-) I updated the PEP: http://hg.python.org/peps/rev/edd8250f6893 > I've made a bunch of small edits (mostly to improve grammar slightly, hope > you don't mind) and committed these to the repo. Thanks, I'm not a native english speaker, so not problem for such edit. > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode437 > pep-0446.txt:437: condition. > As C-F Natali pointed out, this is not actually a problem, because after > fork() > only the main thread survives. Maybe just delete this paragraph? Ok, I didn't know that only one thread survives to fork(). (I read Charles' email, but I forgot to update the PEP.) I simply deleted the paragraph. > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode450 > pep-0446.txt:450: parameter is a non-empty list of file descriptors. > Well, it could pass closefrom() the max of the given list and manually close > the > rest. This would be useful if the system max is large but none of the FDs > given > in the list is. (This would be more complex code but it would address the > issue > for most programs.) This was related to the multi-thread issue, which does not exist, so I also removed this paragraph. Using closefrom() to optimize subprocess is unrelated to this PEP. (And yes, the maximum file descriptor can be huge!) > https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode538 > pep-0446.txt:538: descriptors). > I would say it should not be changed because the default is still better. > :-) (The PEP does not propose to change the default value.) Under Linux, recent versions of the glibc uses non-inheritable FD for internal files. Slowly, more and more libraries and programs will do the same. This PEP is a step in this direction ;-) Victor ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Hi Victor, I have reviewed the PEP and I think it is good. Thank you so much for pushing this topic and for your very thorough review of all the feedback, related issues and so on. It is an exemplary PEP! I've made a bunch of small edits (mostly to improve grammar slightly, hope you don't mind) and committed these to the repo. I've also got a few more comments on the text that I didn't want to commit behind your back; I've written these up in a Rietveld review of the PEP (which you can also use to see exactly what I did already commit). https://codereview.appspot.com/13240043/ Here's a summary of those review changes: https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt File pep-0446.txt (right): https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode25 pep-0446.txt:25: descriptors. I'd add at this point: We are aware of the code breakage this is likely to cause, and doing it anyway for the good of mankind. (Details in the section "Backward Compatibility" below.) https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode80 pep-0446.txt:80: inheritable handles are inherited by the child process. Maybe mention here that this also affects the subprocess module? (You mention it later, but it's important to realize at this point.) https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode437 pep-0446.txt:437: condition. As C-F Natali pointed out, this is not actually a problem, because after fork() only the main thread survives. Maybe just delete this paragraph? https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode450 pep-0446.txt:450: parameter is a non-empty list of file descriptors. Well, it could pass closefrom() the max of the given list and manually close the rest. This would be useful if the system max is large but none of the FDs given in the list is. (This would be more complex code but it would address the issue for most programs.) https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode486 pep-0446.txt:486: * ``socket.socketpair()`` I would call out that dup2() is intentionally not in this list, and add a rationale for that omission below. https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode528 pep-0446.txt:528: by default, but non-inheritable if *inheritable* is ``False``. This might be a good place to explain the rationale for this exception. https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode538 pep-0446.txt:538: descriptors). I would say it should not be changed because the default is still better. :-) -- --Guido van Rossum (python.org/~guido) ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
On Fri, Aug 23, 2013 at 1:30 PM, Charles-François Natali wrote: >> About your example: I'm not sure that it is reliable/portable. I sa >> daemon libraries closing *all* file descriptors and then expecting new >> file descriptors to become 0, 1 and 2. Your example is different >> because w is still open. On Windows, I have seen cases with only fd 0, >> 1, 2 open, and the next open() call gives the fd 10 or 13... > > Well, my example uses fork(), so obviously doesn't apply to Windows. > It's perfectly safe on Unix. But relying on this in UNIX has also been discouraged ever since the dup2() system call was introduced. (I can't easily find a reference about its history but IIRC it is probably as old as UNIX v7 or otherwise BSD 4.x.) >> I'm optimistic and I expect that most Python applications and >> libraries already use the subprocess module. The subprocess module >> closes all file descriptors (except 0, 1, 2) since Python 3.2. >> Developers relying on the FD inheritance and using the subprocess with >> Python 3.2 or later already had to use the pass_fds parameter. > > As long as the PEP makes it clear that this breaks backward > compatibility, that's fine. IMO the risk of breakage outweights the > modicum benefit. I know this will break code. But it is for the good of mankind. (I will now review the full PEP, finally.) -- --Guido van Rossum (python.org/~guido) ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
> About your example: I'm not sure that it is reliable/portable. I sa > daemon libraries closing *all* file descriptors and then expecting new > file descriptors to become 0, 1 and 2. Your example is different > because w is still open. On Windows, I have seen cases with only fd 0, > 1, 2 open, and the next open() call gives the fd 10 or 13... Well, my example uses fork(), so obviously doesn't apply to Windows. It's perfectly safe on Unix. > I'm optimistic and I expect that most Python applications and > libraries already use the subprocess module. The subprocess module > closes all file descriptors (except 0, 1, 2) since Python 3.2. > Developers relying on the FD inheritance and using the subprocess with > Python 3.2 or later already had to use the pass_fds parameter. As long as the PEP makes it clear that this breaks backward compatibility, that's fine. IMO the risk of breakage outweights the modicum benefit. > The subprocess module has still a (minor?) race condition in the child > process. Another C thread can create a new file descriptor after the > subprocess module closed all file descriptors and before exec(). I > hope that it is very unlikely, but it can happen. No it can't, because after fork(), there's only one thread. It's perfectly safe. cf ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Hi, I will try to answer to your worries. Tell me if I should complete the PEP with these answers. 2013/8/23 Charles-François Natali : > Why does dup2() create inheritable FD, and not dup()? Ah yes, there were as section explaining it. In a previous version of the PEP (and its implementation), os.dup() and os.dup2() created non-inheritable FD, but inheritable FD for standard steams (fd 0, 1, 2). I did a research on http://code.ohloh.net/ to see how os.dup2() is used in Python projects. 99.4% (169 projects on 170) uses os.dup2() to replace standard streams: file descriptors 0, 1, 2 (stdin, stdout, stderr); sometimes only stdout or stderr. I only found a short demo script using dup2() with arbitrary file descriptor numbers (10 and 11) to keep a copy of stdout and stderr before replacing them (also with dup2). I didn't find use cases of dup() to inherit file descriptors in the Python standard library. It's the opposite: when os.dup() (or the C function dup() is used), the FD must not be inherited. For example, os.listdir(fd) duplicates fd: a child process must not inherit the duplicated file descriptor, it may lead a security vulnerability (ex: parent process running as root, whereas the child process is running as a different used and not allowed to open the directory). > For example, a lot of code uses the guarantee that dup()/open()... > returns the lowest numbered file descriptor available, so code like > this: > > r, w = os.pipe() > if os.fork() == 0: > # child > os.close(r) > os.close(1) > dup(w) > > *will break* > > And that's a lot of code (e.g. that's what _posixsubprocess.c uses, > but since it's implemented in C it's wouldn't be affected). Yes, it will break. As I wrote in my previous email, we cannot solve all issues listed in the Rationale section of the PEP without breaking applications (or at least breaking backward compatibility). It is even explicitly said in the "Backward Compatibility" section: http://www.python.org/dev/peps/pep-0446/#backward-compatibility "This PEP break applications relying on inheritance of file descriptors." But I also added a hint to fix applications: "Developers are encouraged to reuse the high-level Python module subprocess which handles the inheritance of file descriptors in a portable way." If you don't want to use subprocess, yes, you will have to add "os.set_inheritable(w)" in the child process. About your example: I'm not sure that it is reliable/portable. I saw daemon libraries closing *all* file descriptors and then expecting new file descriptors to become 0, 1 and 2. Your example is different because w is still open. On Windows, I have seen cases with only fd 0, 1, 2 open, and the next open() call gives the fd 10 or 13... I'm optimistic and I expect that most Python applications and libraries already use the subprocess module. The subprocess module closes all file descriptors (except 0, 1, 2) since Python 3.2. Developers relying on the FD inheritance and using the subprocess with Python 3.2 or later already had to use the pass_fds parameter. > Furthermore, many people use Python for system programming, and this > change would be highly surprising. Yes, it is a voluntary design choice (of the PEP). It is also said explicitly in the "Backward Compatibility" section: "Python does no more conform to POSIX, since file descriptors are now made non-inheritable by default. Python was not designed to conform to POSIX, but was designed to develop portable applications." > So no matter what the final decision on this PEP is, it must be kept in mind. The purpose of the PEP is to explain correctly the context and the consequences of the changes, so Guido van Rossum can uses the PEP to make its final decision. >> The programming languages Go, Perl and Ruby make newly created file >> descriptors non-inheritable by default: since Go 1.0 (2009), Perl 1.0 (1987) >> and Ruby 2.0 (2013). > > OK, but do they expose OS file descriptors? Yes: - Perl: fileno() function - Ruby: fileno() method of a file object - Go: fd() method of a file object > Last time, I said that to me, the FD inheritance issue is solved on > POSIX by the subprocess module which passes close_fds. In my own code, > I use subprocess, which is the "official", portable and safe way to > create child processes in Python. Someone using fork() + exec() should > know what he's doing, and be able to deal with the consequences: I'm > not only talking about FD inheritance, but also about > async-signal/multi-threaded safety ;-) The subprocess module has still a (minor?) race condition in the child process. Another C thread can create a new file descriptor after the subprocess module closed all file descriptors and before exec(). I hope that it is very unlikely, but it can happen. It's also explained in the PEP (see "Closing All Open File Descriptors"). I suppose that the race condition explains why Linux still has no closefrom() or nextfd() system calls. IMO the kernel is the bes
Re: [Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Hello, A couple remarks: > The following functions are modified to make newly created file descriptors > non-inheritable by default: > [...] > os.dup() then > os.dup2() has a new optional inheritable parameter: os.dup2(fd, fd2, > inheritable=True). fd2 is created inheritable by default, but non-inheritable > if inheritable is False. Why does dup2() create inheritable FD, and not dup()? I think a hint is given a little later: > Applications using the subprocess module with the pass_fds parameter or using > os.dup2() to redirect standard streams should not be affected. But that's overly-optimistic. For example, a lot of code uses the guarantee that dup()/open()... returns the lowest numbered file descriptor available, so code like this: r, w = os.pipe() if os.fork() == 0: # child os.close(r) os.close(1) dup(w) *will break* And that's a lot of code (e.g. that's what _posixsubprocess.c uses, but since it's implemented in C it's wouldn't be affected). We've already had this discussion, and I stand by my claim that changing the default *will break* user code. Furthermore, many people use Python for system programming, and this change would be highly surprising. So no matter what the final decision on this PEP is, it must be kept in mind. > The programming languages Go, Perl and Ruby make newly created file > descriptors non-inheritable by default: since Go 1.0 (2009), Perl 1.0 (1987) > and Ruby 2.0 (2013). OK, but do they expose OS file descriptors? I'm sure such a change would be fine for Java, which doesn't expose FDs and fork(), but Python's another story. Last time, I said that to me, the FD inheritance issue is solved on POSIX by the subprocess module which passes close_fds. In my own code, I use subprocess, which is the "official", portable and safe way to create child processes in Python. Someone using fork() + exec() should know what he's doing, and be able to deal with the consequences: I'm not only talking about FD inheritance, but also about async-signal/multi-threaded safety ;-) As for Windows, since it doesn't have fork(), it would make sense to make its FD non heritable by default. And then use what you describe here to selectively inherit FDs (i.e. implement keep_fds): """ Since Windows Vista, CreateProcess() supports an extension of the STARTUPINFO struture: the STARTUPINFOEX structure. Using this new structure, it is possible to specify a list of handles to inherit: PROC_THREAD_ATTRIBUTE_HANDLE_LIST. Read Programmatically controlling which handles are inherited by new processes in Win32 (Raymond Chen, Dec 2011) for more information. """ cf ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
[Python-Dev] PEP 446 (make FD non inheritable) ready for a final review
Hi, I know that I wrote it more than once, but I consider that my PEP 446 is now ready for a final review: http://www.python.org/dev/peps/pep-0446/ The implementation is also working, complete and ready for a review. http://hg.python.org/features/pep-446 http://bugs.python.org/issue18571 I run regulary the test suite on my virtual machines: Windows 7 SP1, Linux 3.2, FreeBSD 9 and OpenIndiana (close to Solaris 11), but also sometimes on OpenBSD 5.2. I don't expect major bugs, but you may find minor issues, especially on old operating systems. I don't have access to older systems. *** I collected the list of all threads related to the inheritance of file descriptors on python-dev since january 2013. I counted no more than 239 messages! Thank you all developers who contributed to these discussions and so helped me to write the PEP 433 and the PEP 446, especially Charles-François Natali and Richard Oudkerk! I read again all these messages, and I cannot "go backward" to the PEP 433. There are too many good reasons against adding a global variable (sys.getdefaultcloexec()), and adding a new inheritable parameter without changing the inheritance to non-inheritable does not solve issues listed in the Rationale of the PEP 446. At the beginning of the discussions, most developers already agreed that making file descriptors non-inheritable by default is the best choice. It took me some months to really understand all the consequences of changing the inheritance. I had also many technical issues because each operating system handles the inheritance of file descriptors differently, especially Windows vs UNIX. For atomic flags, there are differences even between minor versions of operating systems. For example, the O_CLOEXEC flag is only supported since Linux 2.6.23. We spend a lot of time to discuss each intermediate solution, but the conclusion is that no compromise can be found on an intermediate solution, only a radical change can solve the problem. [Python-Dev] Add "e" (close and exec) mode to open() (13 messages) Tue Jan 8 2013 http://mail.python.org/pipermail/python-dev/2013-January/123494.html [Python-Dev] Set close-on-exec flag by default in SocketServer (31) Wed Jan 9 2013 http://mail.python.org/pipermail/python-dev/2013-January/123552.html [Python-Dev] PEP 433: Add cloexec argument to functions creating file descriptors (27) Sun Jan 13 2013 http://mail.python.org/pipermail/python-dev/2013-January/123609.html [Python-Dev] Implementation of the PEP 433 (2) Fri Jan 25 2013 http://mail.python.org/pipermail/python-dev/2013-January/123684.html [Python-Dev] PEP 433: Choose the default value of the new cloexec parameter (37) Fri Jan 25 2013 http://mail.python.org/pipermail/python-dev/2013-January/123685.html [Python-Dev] PEP 433: second try (5) Tue Jan 29 2013 http://mail.python.org/pipermail/python-dev/2013-January/123763.html [Python-Dev] Release or not release the GIL (11) Fri Feb 1 2013 http://mail.python.org/pipermail/python-dev/2013-February/123780.html [Python-Dev] Status of the PEP 433? (2) Thu Feb 14 2013 http://mail.python.org/pipermail/python-dev/2013-February/124070.html [Python-Dev] PEP 446: Add new parameters to configure the inherance of files and for non-blocking sockets (24) Thu Jul 4 2013 http://mail.python.org/pipermail/python-dev/2013-July/127168.html [Python-Dev] Inherance of file descriptor and handles on Windows (PEP 446) (41) Wed Jul 24 2013 http://mail.python.org/pipermail/python-dev/2013-July/127509.html http://mail.python.org/pipermail/python-dev/2013-August/127791.html [Python-Dev] PEP 446: Open issues/questions (29) Sun Jul 28 2013 http://mail.python.org/pipermail/python-dev/2013-July/127626.html http://mail.python.org/pipermail/python-dev/2013-August/127728.html [Python-Dev] (New) PEP 446: Make newly created file descriptors non-inheritable (8) Tue Aug 6 2013 http://mail.python.org/pipermail/python-dev/2013-August/127805.html [Python-Dev] PEP 446: issue with sockets (9) Wed Aug 21 2013 http://mail.python.org/pipermail/python-dev/2013-August/128045.html Victor ___ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
