The specific issue this is creating is that a malicious user could use this socket in a subprocess which is started from a library (ie. I'm using a .so, which calls fork/exec).

A second failure mode is starting a daemon from withing, say, a django application. Djano opens a TCP listening socket, then starts up a daemon to provide some sort of service in the background. The daemon keeps running and inherits the socket. Now you restart the django app.

It refuses to start ! Why ? Because the socket was inherited, the listening socket isn't actually closed, and this results in the socket being stuck in CLOSE_WAIT as long as the daemon is running.

It seems to me that it is almost never the case that you'd want a TCP listening socket to be preserved across exec, and not setting this flag should thus be considered a bug for 2 reasons :

1) it results in accidental disclosure of information that shouldn't be exposed in certain cases.
2) it can result in denial of service

Solution : 

update SocketServer.py :
  in the class TCPServer
  add the following 2 lines in __init__ after self.socket = socket( ...:
    flags = fcntl.fcntl(self.socket, fcntl.F_GETFD)
    fcntl.fcntl(self.socket, fcntl.F_SETFD, flags | fcntl.FD_CLOEXEC)

Hello Christophe,

First and foremost, I think that the FD_CLOEXEC approach is terminally broken, as it should have been the default in Unix. Now, we're stuck with this bad design.
But we can't simply change the default to FD_CLOEXEC, for two reasons:
- we can't silently change the Unix semantics
- this is going to break some applications: for example, FD inherited across exec is used by super servers such as inetd, and there are others very legitimate uses

>  in the class TCPServer
>  add the following 2 lines in __init__ after self.socket = socket( ...:
>    flags = fcntl.fcntl(self.socket, fcntl.F_GETFD)
>    fcntl.fcntl(self.socket, fcntl.F_SETFD, flags | fcntl.FD_CLOEXEC)

There are at least two problems with this approach:
1) there's a race between the socket creation and the call to fcntl
2) accept doesn't necessarily inherit the FD_CLOEXEC flag

1) can be fixed on systems that support it through SOCK_CLOEXEC
2) can be fixed on systems that support it through accept4(), but it seems to break badly on some systems, see issue #10115

But I think this is a perfectly legitimate request, so one approach to tackle this problem could be:
- since accept4() seems to fail so badly in some configurations, the only portable and reliable choice left is probably to call accept() then fcntl(FD_CLOEXEC) (there's a race, but it's better than nothing). We might reconsider this syscall in a couple years when we're sure it's implemented correctly
- in the socketserver module, add a new set_socket_cloexec attribute to BaseServer, which would do the right thing (i.e. create the socket with SOCK_CLOEXEC if available, otherwise call fcntl(FD_CLOEXEC)), and in TCPServer, call fcntl(FD_CLOEXEC) after accept.

That way, this would at least fix the problem for people using the socketserver module. People using sockets directly of course have the option of using SOCK_CLOEXEC and fcntl(FD_CLOEXEC) explicitely in their code.

Gregory, any thoughts on this?

I realize this bugreport cannot fix 35 years of a bad design decision in
linux. That's not the intention (that's a gordian knot I *will* be keeping a
safe distance from). The intention is to create a saner default situation
for most python programs.

Christophe

2011/5/20 Charles-François Natali <report@bugs.python.org>

>
> Charles-François Natali <neologix@free.fr> added the comment:
>
> Hello Christophe,
>
> First and foremost, I think that the FD_CLOEXEC approach is terminally
> broken, as it should have been the default in Unix. Now, we're stuck with
> this bad design.
> But we can't simply change the default to FD_CLOEXEC, for two reasons:
> - we can't silently change the Unix semantics
> - this is going to break some applications: for example, FD inherited
> across exec is used by super servers such as inetd, and there are others
> very legitimate uses
>
> >  in the class TCPServer
> >  add the following 2 lines in __init__ after self.socket = socket( ...:
> >    flags = fcntl.fcntl(self.socket, fcntl.F_GETFD)
> >    fcntl.fcntl(self.socket, fcntl.F_SETFD, flags | fcntl.FD_CLOEXEC)
>
> There are at least two problems with this approach:
> 1) there's a race between the socket creation and the call to fcntl
> 2) accept doesn't necessarily inherit the FD_CLOEXEC flag
>
> 1) can be fixed on systems that support it through SOCK_CLOEXEC
> 2) can be fixed on systems that support it through accept4(), but it seems
> to break badly on some systems, see issue #10115
>
> But I think this is a perfectly legitimate request, so one approach to
> tackle this problem could be:
> - since accept4() seems to fail so badly in some configurations, the only
> portable and reliable choice left is probably to call accept() then
> fcntl(FD_CLOEXEC) (there's a race, but it's better than nothing). We might
> reconsider this syscall in a couple years when we're sure it's implemented
> correctly
> - in the socketserver module, add a new set_socket_cloexec attribute to
> BaseServer, which would do the right thing (i.e. create the socket with
> SOCK_CLOEXEC if available, otherwise call fcntl(FD_CLOEXEC)), and in
> TCPServer, call fcntl(FD_CLOEXEC) after accept.
>
> That way, this would at least fix the problem for people using the
> socketserver module. People using sockets directly of course have the option
> of using SOCK_CLOEXEC and fcntl(FD_CLOEXEC) explicitely in their code.
>
> Gregory, any thoughts on this?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue12107>
> _______________________________________
>

> That's not the intention (that's a gordian knot I *will* be keeping
> a
> safe distance from). The intention is to create a saner default situation
> for most python programs.

I understand what you're saying, and I agree with you on the principle.
But the point I was making is that we can't make this the "default
situation", for the reasons explained above (we can't change the
default socket semantics, and it's going to break some applications,
which is unacceptable).
That's why I was suggesting to add this as an option to the
socketserver module (since that's the piece of code you were referring
to).
The Python interpreter is not a simple standalone library/application,
we can't just make those kind of changes.

P.S.: please avoid sending html messages, they result in a spurious
"unnamed" file being attached.

It would already be a nice piece of progress if you could request the
SO_CLOEXEC (with fallback to FD_CLOEXEC), say, in the constructor, or even
with a module variable. I hope at least this change can make it in, so that
we have a decent in-python solution that can be used from within, say,
django.

Christophe

On Sat, May 21, 2011 at 1:24 AM, Antoine Pitrou <report@bugs.python.org>wrote:

>
> Changes by Antoine Pitrou <pitrou@free.fr>:
>
>
> Removed file: http://bugs.python.org/file22038/unnamed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue12107>
> _______________________________________
>

The request is reasonable, but the best bet is for someone interested to provide a patch. See http://docs.python.org/devguide/ if you aren't acquainted with the development process.

$ cat test_sock.py 
import socket
import fcntl


with socket.socket(socket.AF_INET, socket.SOCK_STREAM|socket.SOCK_CLOEXEC) as s:
    print(bool(fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC))

$ ./python test_sock.py 
True

So, SOCK_CLOEXEC is available.
Note that I don't like the idea of falling back to FD_CLOEXEC since
it's not atomic, and some people might rely on this.
Can we close this issue?

> So, SOCK_CLOEXEC is available.
> Note that I don't like the idea of falling back to FD_CLOEXEC since
> it's not atomic, and some people might rely on this.
> Can we close this issue?

Well, this is apparently a feature request for socketserver.TCPServer.
I don't see any problem in adding a best-effort option to add the cloexec flag, possibly atomically, and fall back on FD_CLOEXEC.

People who "rely on this" can only do it if their system supports it anyway.

For the record, the best effort approach is already used in subprocess (check subprocess_cloexec_pipe() in Modules/_posixsubprocess.c).

Speaking of which, the os module could expose the pipe2() function.

> Well, this is apparently a feature request for socketserver.TCPServer.

Honestly, I'm not sure what this request is about.
The original request seemed to imply this should be made the default. I don't agree, and think this should be made optional. Also, the title is much more general: "TCP listening sockets created without FD_CLOEXEC flag".

> I don't see any problem in adding a best-effort option to add the cloexec flag, possibly atomically, and fall back on FD_CLOEXEC.
> People who "rely on this" can only do it if their system supports it anyway.

I wasn't clear. I thought we were talking about the socket constructor. In that case, yes, we could do that for socketserver, but with an explicit warning (because with a cloexec flag to a constructor, people will unconsciously expect it to behave like SOCK_CLOEXEC and be atomic).

> Speaking of which, the os module could expose the pipe2() function.

I'll work on a patch.

See also #5715 which asks for something similar.

"Note that I don't like the idea of falling back to FD_CLOEXEC since
it's not atomic, and some people might rely on this."

There is a similar question (atomic or best effort) for the #16860 issue. It looks like more people prefer the best effort option.

IMO most people prefer to use the best available method to set the close-on-exec flag, but doesn't really care if the method is not atomic (and the OS doesn't provide an atomic function).

If someone cannot accept the best effort option: I guess that it is still possible to override TCPServer constructor to ensure that the action is atomic.

"It would already be a nice piece of progress if you could request the
SO_CLOEXEC (with fallback to FD_CLOEXEC), say, in the constructor, or even with a module variable."

Yes, close-on-exec flag should be configurable, as TCPServer.allow_reuse_address for example. I like the class attribute, something like TCPServer.close_on_exec, False by default, for example.

--

"I realize this bugreport cannot fix 35 years of a bad design decision in linux."

Well... Ruby made a brave choice :-) Ruby (2.0?) does set
close-on-exec flag on *ALL file descriptors (except 0, 1, 2) *by
default*:
http://bugs.ruby-lang.org/issues/5041

This change solves the problem of having to close all file descriptor
after a fork to run a new program (see closed Python issues #11284 and
#8052)... if you are not using C extensions creating file descriptors?

Ruby applications relying on passing FD to child processes have to
explicitly disable close-on-exec the flag: it was done in Unicorn for
example.

"But we can't simply change the default to FD_CLOEXEC, for two reasons:
- we can't silently change the Unix semantics
- this is going to break some applications: for example, FD inherited across exec is used by super servers such as inetd, and there are others very legitimate uses"

Yes, changing the default value of the flag (enable close-on-exec by default) will break some applications. But if we chose to do it, developers of these applications can still disable close-on-exec (because the option must be configurable) to fix their application for Python 3.4.

Oh, by the way: I never understood why SimpleXMLRPCServer does set FD_CLOEXEC (since the issue #1222790), whereas TCPServer doesn't.

socketserver_close_on_exec.patch:
 - Add TCPServer.close_on_exec class attribute: True by default if the fcntl module is present, False otherwise
 - Use SOCK_CLOEXEC if present, fcntl() + FD_CLOEXEC otherwise

Even if SOCK_CLOEXEC is present, fcntl() is used to check the flag "works" (if FD_CLOEXEC was set). If SOCK_CLOEXEC works, fcntl() will not be called anymore, otherwise we fall back to fcntl() + FD_CLOEXEC. I implemented this fallback for Linux older than 2.6.27. I don't know if SOCK_CLOEXEC is simply ignored, as Linux did for O_CLOEXEC, but I don't have such old Linux version to test.

I chose to set close-on-exec flag *by default*. If we chose to disable it by default, the following changes should be done on my patch:
 - TCPServer.close_on_exec : "close_on_exec = (fcntl is not None)" => "close_on_exec = False"
 - SimpleXMLRPCServer: add close_on_exec class attribute, "close_on_exec = (fcntl is not None)" (and restore try/except ImportError for fcntl)

> There is a similar question (atomic or best effort) for the #16860 issue. It looks like more people prefer the best effort option.

Oops, it's the issue #16850: Add "e" mode to open(): close-and-exec (O_CLOEXEC) / O_NOINHERIT

Oh, I completly forgot the client socket for TCP servers: updated patch. The patch version 2 uses fcntl()+FD_CLOEXEC after accept(). It is not atomic, but I prefer to leave to problem to issue #10115:

"can be fixed on systems that support it through accept4(), but it seems to break badly on some systems, see issue #10115"

> "I realize this bugreport cannot fix 35 years of a bad design decision in linux."
>
> Well... Ruby made a brave choice :-) Ruby (2.0?) does set
> close-on-exec flag on *ALL file descriptors (except 0, 1, 2) *by
> default*:
> http://bugs.ruby-lang.org/issues/5041
>
> This change solves the problem of having to close all file descriptor
> after a fork to run a new program (see closed Python issues #11284 and
> #8052)... if you are not using C extensions creating file descriptors?

> "But we can't simply change the default to FD_CLOEXEC, for two reasons:
> - we can't silently change the Unix semantics
> - this is going to break some applications: for example, FD inherited across exec is used by super servers such as inetd, and there are others very legitimate uses"
>
> Yes, changing the default value of the flag (enable close-on-exec by default) will break some applications. But if we chose to do it, developers of these applications can still disable close-on-exec (because the option must be configurable) to fix their application for Python 3.4.

Changing all FDs to close-on-exec by default would be a bold move (and
even just for SocketServer).
It ought to be default, but it's going to break some applications.
That's definitely something which needs discussion (at not only on the
tracker, I would could bring it up on python-dev).

> That's definitely something which needs discussion
> (at not only on the tracker, I would could bring it up on python-dev).

I agree, I started a thread on python-dev mailing list:
https://mail.python.org/pipermail/python-dev/2013-January/123552.html

Sockets are now created non-inheritable in Python 3.4. See the PEP 446 for more information (PEP implemented in the issue #18571).