problems with file descriptors in redir_fork

[CristiCimpianu]

Hi guys,

I'm kind of stuck trying to enable redirssl due to a problem I tracked down in redir_fork. It goes as follows:

1. redir_accept creates a new_socket that it passes to redir_fork
2. redir_fork closes fd 0 and fd 1 and duplicates new_socket to 0 and 1
3. redir_accept now calls redir_main passing 0 and 1 as input fds.

In my case the chilli process has the following fds:

[root@arena_cristi init.d]# ls -l /proc/19088/fd
total 0
lrwx------ 1 root root 64 Dec  4 17:02 0 -> socket:[535197]
lrwx------ 1 root root 64 Dec  4 17:02 1 -> /dev/net/tun
lrwx------ 1 root root 64 Dec  4 17:02 10 -> anon_inode:[eventpoll]
lr-x------ 1 root root 64 Dec  4 17:02 11 -> pipe:[535229]
l-wx------ 1 root root 64 Dec  4 17:02 12 -> pipe:[535229]
lrwx------ 1 root root 64 Dec  4 17:02 13 -> socket:[535230]
lrwx------ 1 root root 64 Dec  4 17:02 2 -> socket:[535204]
lrwx------ 1 root root 64 Dec  4 17:02 3 -> socket:[535215]
lr-x------ 1 root root 64 Dec  4 17:02 4 -> /dev/urandom
lrwx------ 1 root root 64 Dec  4 17:02 5 -> socket:[535216]
lrwx------ 1 root root 64 Dec  4 17:02 6 -> socket:[535217]
l-wx------ 1 root root 64 Dec  4 17:02 7 -> pipe:[85431]
lrwx------ 1 root root 64 Dec  4 17:02 8 -> socket:[535218]
lrwx------ 1 root root 64 Dec  4 17:02 9 -> socket:[535219]

Now a few things I noticed:

1. if I start chilli with -d all messages the child sends to syslog are injected into the http socket and I see them in my browser along with the rest of the html code sent there. in case it's a https request the browser simply timeouts.
2. if I start without -d the http request goes fine, while https request displays the invalid certificate warning and after accepting it the browser says secure connection failed.
3. if I comment out the fd duplication code in redir_fork and I make redir_main take new_socket both http and https requests work fine.

From these I deduce that fd 0 in my case is actually the syslog socket, while fd 1 is used by the child to send some data back to the client so closing them is a bad idea. However redir_fork is used in 4 more places and there's also the case with _options.uamui being called and I assume it expects the http(s) socket to be linked to its stdin and stdout. So far I can't think of a clean solution to cover all cases. I will continue trying to search for one next week, but I thought some advice from you will surely help me out.

The minimal set of working changes I did is the following:

[root@localhost coova-chilli-1.3.1.3]# git diff
diff --git a/src/redir.c b/src/redir.c
index a4c0c19..b7162f0 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -3172,9 +3172,10 @@ int redir_accept(struct redir_t *redir, int idx) {
     execv(*binqqargs, binqqargs);

   } else {
+    int ret;
+    ret = redir_main(redir, new_socket, new_socket, &address, &baddress, idx, 0);
     safe_close(new_socket);
-    return redir_main(redir, 0, 1, &address, &baddress, idx, 0);
-
+    return ret;
   }

   safe_close(new_socket);
@@ -3274,8 +3275,8 @@ pid_t redir_fork(int in, int out) {
     }

 #if defined(F_DUPFD)
-    if (fcntl(in,F_GETFL,0) == -1) return -1; safe_close(0);
-    if (fcntl(in,F_DUPFD,0) == -1) return -1;
+//    if (fcntl(in,F_GETFL,0) == -1) return -1; safe_close(0);
+//    if (fcntl(in,F_DUPFD,0) == -1) return -1;
     if (fcntl(out,F_GETFL,1) == -1) return -1; safe_close(1);
     if (fcntl(out,F_DUPFD,1) == -1) return -1;
 #else

thanks in advance, Cristian.

[gbaligh]

Did you tried to run coova in foreground mode ? using -f argument ? It may be the same problem as in #106 . The problem is related to when closing all output/input in daemon mode, i guess.

[CristiCimpianu]

@gbaligh Running in foreground mode will probably hide this issue, which btw is probably also linked to https://github.com/coova/coova-chilli/issues/51. The patch you applied still has a problem that it closes stdin and links new_socket to stdin. if any logs will be printed by the child they will be sent to new_socket. I think the only 'clean' solution would be to separate the fd duplication from the redir_fork function and use that only when calling _options.uamui scripts. Also using the FD_CLOEXEC flag on most descriptors may be useful.

[gbaligh]

Hi @CristiCimpianu , Thanks for your indication, but it's not me who closed the issue ;) And I have no right to reopen it, if @wlanmac can reopen it it will be nice.

I puled the first part of your work, because it's me who did the safe_close(new_socket); before redir_main(). The second par related to stdin and stdout. if it's working in foreground mode, I think it's better to check this part in chilli.c

[ynezz]

@CristiCimpianu This is still happening on the latest Git master? It's not clear to me which version are you using.

[CristiCimpianu]

@ynezz I'm always rebasing on the latest master. As long as there is no logging from the child process everything is fine. Once you enable logging in the fork child my rsyslog sends messages only to stdout (which is now the http/s socket) and breaks the connection. In the fork parent all rsyslog messages go to /var/log/messages. I removed also the LOG_PERROR flag from the syslog connection with no luck.

[ynezz]

@CristiCimpianu Hm strange, I've redirssl + uamuissl enabled and it works for me.

I think, that if you're able to compile it yourself and you can find a working tag/revision in the past which was working for you, then the fastest way to find the bug and fix it is to use git bisect.

[CristiCimpianu]

@ynezz do you have coova running with any logs (-d flag, debugfacility, loglevel)? Without logging it works for me too.