close std-filehandles when daemonize

[ftasnetamot]

As described in #467, sslh does not close the std-filehandles when daemonizing. Closing those filehandles sslh.main.c and corrected a comment in sslh-fork.c

[ftasnetamot]

And: It was not reported earlier, as systemd is nowadays everywhere. And as I discovered, it is still grabbing filehandles, when you think, that you have untied it for a single program :-(

[ftasnetamot]

Had now time to reread Stevens, and it looks like, I should not close the dupped dhandle. The reason why to dup2-ing is:

These guarantees that this common descriptors are open and the kernel just discards anything written to ... The reason ... is, that any library function called from the daemon that assumes it can read from stdout or stderr or write to stdin will not fail. Looks like, I followed a good advice, but broke it with an close(). I assume, that I changed my old program some time later, and found an open handle, and closed that (as there was no hint in a comment, why this should stay open).

So its upon you. I guess, that the risk, that library function will talk back is minimal, and with my solution i broke it, as your solution will break it.

Shall I create a new PR and keeping this handle open, or will you vote for saving handles an close it your way?

[ftasnetamot]

Did now some more tests:

When just closing the three filehandles with

  close(STDIN_FILENO);
  close(STDOUT_FILENO);
  close(STDERR_FILENO); 

the output of ls -lad /proc/1359[45]/fd/* looks like:

l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/1 -> /lib
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/2 -> /usr/lib
lrwx------ 1 root root 64 24. Aug 14:30 /proc/13594/fd/3 -> 'socket:[393977]'
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/4 -> /etc/ld.so.cache
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/5 -> /etc/hosts
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/6 -> /run/resolvconf/resolv.conf
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/7 -> /etc/nsswitch.conf

Now, as all handles where closed: stdin and stdout where replaced by file-access, which used other filehandles before. This can lead for sure to confusion, if any other program still thinks, it can expect anything meaningfull from those handles. Instead of stderr it is now the listening socket! So this describes the problem, which should be avoided with the dup2() solution.

As a next test I omitted the close to new_fd from my my solution, but that leads to:

ls -lad /proc/1429[01]/fd/*
lrwx------ 1 root root 64 24. Aug 14:42 /proc/14290/fd/0 -> /dev/null
lrwx------ 1 root root 64 24. Aug 14:42 /proc/14290/fd/1 -> /dev/null
lrwx------ 1 root root 64 24. Aug 14:42 /proc/14290/fd/2 -> /dev/null
lrwx------ 1 root root 64 24. Aug 14:42 /proc/14290/fd/3 -> 'socket:[394895]'
lrwx------ 1 root root 64 24. Aug 14:42 /proc/14290/fd/4 -> /dev/null
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/6 -> /lib
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/7 -> /usr/lib
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/8 -> /etc/ld.so.cache
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/9 -> /etc/hosts
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/10 -> /run/resolvconf/resolv.conf
l--------- 1 root root 64 24. Aug 14:42 /proc/14290/fd/11 -> /etc/nsswitch.conf

which produced another filehandle, exactly that for new_fp, which was not closed. So my close() is correct, as it only closes the helper-handle, all duplicates are real own open filehandles. As close() was used for opening new_fd, the overhead for buffering etc. compared to fopen() is reduced.

While doing this tests, older memories are coming back. I added the close, just to reduce the filehandle count, as an open helper handle is really not needed! Seeing the behaviour from systemd, grabbing the filehandles of sslh, (described in #467), I am sure, that my proposal ist the best solution.

So my initial PR is still valid, as that makes sure, that stdin/stdout/stderr are not leading to any confusion. However, if you agree to this solution, I should add another pair of braces to the if-condition, to avoid the warning, I am now getting with a more recent compiler: warning: suggest parentheses around assignment used as truth value [-Wparentheses]

The curiosity detected by this test is, that stdin gets not replaced, after it is closed. That seems to be related, that stdin is open by default and no filehandle with the number "0" gets assigned, as in those cases this looks like an error! So closing stdin may be a solution, so save filehandles.

[ftasnetamot]

I am now researching, why I have those additional filehandles open, like:

l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/1 -> /lib
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/2 -> /usr/lib
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/4 -> /etc/ld.so.cache
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/5 -> /etc/hosts
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/6 -> /run/resolvconf/resolv.conf
l--------- 1 root root 64 24. Aug 14:30 /proc/13594/fd/7 -> /etc/nsswitch.conf

The binary, I have from my first tests after creating the PR consumes only 4 handles: stdin,stdout,stderr and the listening socket. Why does the current binary consumes 6 handles more?

I went now back to my older system with gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.2) and when I compile the same code there, sslh has only 4 filehandles open.

My setup is now the orginal checkout of #468 and in the Makefile I have changed the following two Options:

override undefine HAVE_LANDLOCK
USELIBCAP=1

My new systems has: gcc (Debian 12.2.0-14) 12.2.0

Realising this, I remember #456. This makes a huge difference, having six handles more, as it looks for dns issues and some other system stuff.

[ftasnetamot]

Now tested Debian 11 (bullseye) Linux debian11 5.10.0-32-amd64

gcc (Debian 10.2.1-6) 10.2.1 20210110 only four/five handles

Same with the package sslh sslh 1.20-1

[ftasnetamot]

Now tested Ubuntu 24-04 Server Linux ubuntu24-04-server 6.8.0-41-generic gcc: gcc (Ubuntu 13.2.0-23ubuntu4) 13.2.0

11 handles!

However the sslh from repository (sslh-fork 1.22c-1) has only 5 handles!

[ftasnetamot]

Looks like, that with newer c-compilers there are some more options needed to reduce the overhead of (dns based??) handles.

Also the newer compilers give much more warnings about unclear code situations, have not yet digged into that. I am curious why the self-compiled binary consumes more filehandles

[ftasnetamot]

Compiling under Ubuntu 22.04 (5.15.0-119-generic) with gcc gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0 is also ok with filehandles!

[ftasnetamot]

OK, to summarize: Up to the gcc-chain 11, the binary does not use additional file handles. With version 12 and 13 it does! However, from version 11 on the big amount on additional warnings appears. I will try to figure out, what changed in the compile chain in between 11 and 12

[ftasnetamot]

Ok, found the solution: Looks like, that override undefine does not longer work. Also defining _HAVE_LANDLOCK 0_ in Makefile does not work. If I remove HAVE_LANDLOCK completely from config.h, the application gets compiled without landlock.

HOWEVER: It looks for me, that the landlock implementation seems to be faulty, not closing the connections. I have not worked with landlock up to now, but it makes no sense at all, to keep all the directories and filehandles open, you might want to use at some place in your code. This gives for landlock users 6 additional filehandles for each forked process!

Those six handles are exactly those directories and files, you are adding in landlock.c

For me it looks like, you need a line:
close(fd);
in line 92 of landlock.c

Ok, expect an updated PR from me:

• changing msg_config to msg_system_error
• adding another pair of braces around the if statement in my PR, to make newer gcc happy
• keep dup2() and close(new_fd)
• still thinking about closing stdin, as this really saves one handle and no problem could occur as this handle will not be reused.
• the other two handles must stay open bmho, especially when watching, how systemd is just catching them. Depending for what the daemon is using one of those handles, this could be hard to track, whats going on.
• Adding an hint into the compiling docu, that with certain compilers "HAVE_LANDLOCK" must disappear from config.h
• adding the missing close() to landlock.c
• correcting the last comment in sslh-select about where to find main()

So much from me for this day

[ftasnetamot]

I have pushed my changes, but I am still in testing! I will tell separately, when I think, everything is ok with PR

[ftasnetamot]

OK, staying conservative is much better. stdin need to stay open, as on two of my testing systems I received errors, because my own new_fd got the filehandle zero, and therefore the condition, checking if opening /dev/null failed ;-) I keep compiling & testing as well on arm64, FreeBSD-14.0-CURRENT-amd64

[ftasnetamot]

still testing, right now everything looks as expected. I did some first additions to the man-page. You should add an redirect from http://www.rutschle.net/tech/sslh to the github project, as this url is still part of thousands systems, but currently delivering an 404. I changed the url in the current pod document.

[ftasnetamot]

From my point of view, everything looks ok. If you have no further question feel free to merge.

[ftasnetamot]

I was the last days debugging a problem with exim, and looked in the exim source, after I detected, that exim is assigning /dev/null to stdin,stdout,stderr. I found the following snippet

[yrutschle]

Thanks for the PR (and in-depth discussion :) ).

• I'm sending a question to the landlock people whether it's ok to close the filehandles. The landlock manpage does not discuss this, and back when I wrote that code I was under the impression I needed to keep them open.
• I'm moving the code to its own function and (hopefully) clarified the comment from what you discussed here, and the Exim comment.
• I've added the redirect on the Web site. Somehow I thought it was already in place...