close std-filehandles when daemonize #468
Conversation
sslh-main.c
Outdated
| dup2 (newfd, STDERR_FILENO); | ||
| close(newfd); | ||
| } else { | ||
| print_message(msg_config, "Error closing standard filehandles for background daemon\n"); |
There was a problem hiding this comment.
I think this more of a msg_system_error, I don't see how this could fail...
| // close stdin, stderr, stdout | ||
| int newfd; | ||
| if (newfd = open("/dev/null", O_RDWR)) { | ||
| dup2 (newfd, STDIN_FILENO); |
There was a problem hiding this comment.
I am curious of why you're doing this. Intuitively I would have simply
close(fileno(stdin));
close(fileno(stdout));
close(fileno(stderr));
(which is how we do it to close stderr when using inetd)
There was a problem hiding this comment.
I remembered this construct from Stevens Network Programming, but I have to reread to give more arguments.
Just copied from some older simple programs, I wrote decades ago
There was a problem hiding this comment.
But I bet, the other way is also working.
|
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 :-( |
|
Had now time to reread Stevens, and it looks like, I should not close the dupped dhandle.
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? |
|
Did now some more tests: When just closing the three filehandles with the output of 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: 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! 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: 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! |
|
I am now researching, why I have those additional filehandles open, like: The binary, I have from my first tests after creating the PR consumes only 4 handles: stdin,stdout,stderr and the listening socket. I went now back to my older system with My setup is now the orginal checkout of #468 and in the Makefile I have changed the following two Options: My new systems has: 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. |
|
Now tested Debian 11 (bullseye)
Same with the package sslh |
|
Now tested Ubuntu 24-04 Server 11 handles! However the sslh from repository ( |
|
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 |
|
Compiling under Ubuntu 22.04 ( |
|
OK, to summarize: Up to the gcc-chain 11, the binary does not use additional file handles. With version 12 and 13 it does! |
|
Ok, found the solution: Looks like, that override undefine does not longer work. 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: Ok, expect an updated PR from me:
So much from me for this day |
|
I have pushed my changes, but I am still in testing! |
|
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 ;-) |
|
still testing, right now everything looks as expected. |
|
From my point of view, everything looks ok. If you have no further question feel free to merge. |
|
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 |
|
Thanks for the PR (and in-depth discussion :) ).
|
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