Closing TCP server sockets on fork

[edzius]

Hi,

I want to suggest to add FD_CLOEXEC flag on TCP server sockets, to automatically close sockets on fork. I actually don't know if it left on purpose or it is a bug but in common use case it makes sense to close server sockets of forked process. I noticed this bug/feature while using luasockets v2.0.2 with xavante and wsapi.

My use case:

• xavante receives request
• wsapi handler processes request and forks process
• forked process lauches script that reloads xavante
• forked process exits

Forked process server sockets still open and when starting xavante again i get 'address already in use' error.

Here is my patch that i used on luasockets-2.0.2:

diff --git a/lua-modules/luasocket-2.0.2/src/socket.h b/lua-modules/luasocket-2.0.2/src/socket.h
index 656c7f5..cd81350 100644
--- a/lua-modules/luasocket-2.0.2/src/socket.h
+++ b/lua-modules/luasocket-2.0.2/src/socket.h
@@ -48,6 +48,7 @@ int socket_recvfrom(p_socket ps, char *data, size_t count,

 void socket_setnonblocking(p_socket ps);
 void socket_setblocking(p_socket ps);
+void socket_set_coe(p_socket ps);

 int socket_waitfd(p_socket ps, int sw, p_timeout tm);
 int socket_select(t_socket n, fd_set *rfds, fd_set *wfds, fd_set *efds,
diff --git a/lua-modules/luasocket-2.0.2/src/tcp.c b/lua-modules/luasocket-2.0.2/src/tcp.c
index 6b8a79b..ab4b219 100644
--- a/lua-modules/luasocket-2.0.2/src/tcp.c
+++ b/lua-modules/luasocket-2.0.2/src/tcp.c
@@ -199,6 +199,11 @@ static int meth_bind(lua_State *L)
         lua_pushstring(L, err);
         return 2;
     }
+    /*
+     * XXX: need to close socket on exec. Requred for xavante
+     * 'address already in use' issue. Described in #2947
+     */
+    socket_set_coe(&tcp->sock);
     lua_pushnumber(L, 1);
     return 1;
 }
diff --git a/lua-modules/luasocket-2.0.2/src/usocket.c b/lua-modules/luasocket-2.0.2/src/usocket.c
index 70c6e1e..57cedbb 100644
--- a/lua-modules/luasocket-2.0.2/src/usocket.c
+++ b/lua-modules/luasocket-2.0.2/src/usocket.c
@@ -320,6 +320,15 @@ void socket_setnonblocking(p_socket ps) {
 }

 /*-------------------------------------------------------------------------*\
+* Put socket to close on exec
+\*-------------------------------------------------------------------------*/
+void socket_set_coe(p_socket ps) {
+    int flags = fcntl(*ps, F_GETFD);
+    flags |= FD_CLOEXEC;
+    fcntl(*ps, F_SETFD, flags);
+}
+
+/*-------------------------------------------------------------------------*\
 * DNS helpers
 \*-------------------------------------------------------------------------*/
 int socket_gethostbyaddr(const char *addr, socklen_t len, struct hostent **hp) {
diff --git a/lua-modules/luasocket-2.0.2/src/wsocket.c b/lua-modules/luasocket-2.0.2/src/wsocket.c
index 6022565..fe4d9ec 100644
--- a/lua-modules/luasocket-2.0.2/src/wsocket.c
+++ b/lua-modules/luasocket-2.0.2/src/wsocket.c
@@ -306,6 +306,12 @@ void socket_setnonblocking(p_socket ps) {
 }

 /*-------------------------------------------------------------------------*\
+* Put socket to close on exec
+\*-------------------------------------------------------------------------*/
+void socket_set_coe(p_socket ps) {
+}
+
+/*-------------------------------------------------------------------------*\
 * DNS helpers
 \*-------------------------------------------------------------------------*/
 int socket_gethostbyaddr(const char *addr, socklen_t len, struct hostent **hp) {
-- 
1.7.10.4

[sam-github]

seems reasonable, or else it should be an option. note that it is close on "exec" not close on "fork"

[edzius]

Oh, you are right.. i missed that. Yes it could be an option, however i can't figure out any scenario that these old socket descriptors would be used in switched process after exec()

[diegonehab]

Is there any reason not to use the

res = ioctl(fd, FIOCLEX, 0);

call instead?

[diegonehab]

We should probably make this an option. We can set it by default on all sockets. Windows doesn't seem to have an equivalent function. Is there similar funcionality on Windows?

[hishamhm]

Was this ever addressed? I just bumped into a similar issue.