Nicolas Williams wrote:
> On Wed, Sep 02, 2009 at 07:10:35PM +0200, Vladimir Kotal wrote:
>> http://cr.opensolaris.org/~vkotal/daemon_libc-4471189.onnv/
>
> - Could you make daemon() replace fildes 0, 1 and 2 the way the ssh
> version (from OpenBSD) did it: open /dev/null, dup2() into 0, 1 and
> 2, then close the /dev/null fildes. Also, please check for errors
> when opening /dev/null for this.
First the obvious stuff:
Checking the return value of open() is useful without any dispute given
PSARC/2009/378.
I think that in case noclose is 0 and one of the open()/dup2() fails
then daemon() should return -1 to signal to the caller it was not able
to perform its job as requested.
Now, there is this "put as little potential failure points into the path
as possible versus if we fail it should be as graceful as possible"
sort-of-dispute :)
Although I liked the solution without the extra descriptor better, given
the comment about signal handlers I am more inclined towards the
open+dup2-only solution. In that case it would be nice to handle the
dup2() failures so in case the process already closed any of
STD{IN,OUT,ERR}_FILENO descriptors (before the call) daemon() should not
attempt to call dup2() needlessly. When handling failures, attempt to
rollback naturally comes to mind:
int fd, fd1;
...
if (noclose == 0) {
/*
* Missing the PRIV_FILE_READ privilege may prevent the
* opening of /dev/null to succeed.
*/
if ((fd = open("/dev/null", O_RDWR)) < 0)
return (-1);
/*
* If any of the functions fails it should have as little
* impact on the caller as possible.
*/
if ((fd != STDIN_FILENO) && dup2(fd, STDIN_FILENO) < 0) {
close(fd);
/*
* even if fd was STDOUT_FILENO or STDERR_FILENO
* we want to leave those untouched (least
* surprise).
*/
return (-1);
}
if ((fd != STDOUT_FILENO) && dup2(fd, STDOUT_FILENO) < 0) {
close(fd);
/* try to reopen stdin, if it was redirected */
if (fd != STDIN_FILENO) {
fd1 = open("/dev/stdin", O_RDWR);
dup2(fd1, 0);
close(fd1);
}
return (-1);
}
if ((fd != STDERR_FILENO) && dup2(fd, STDERR_FILENO) < 0) {
close(fd);
/* try to reopen std{in,out} if redirected */
if (fd != STDIN_FILENO) {
fd1 = open("/dev/stdin", O_RDWR);
dup2(fd1, STDIN_FILENO);
close(fd1);
}
if (fd != STDOUT_FILENO) {
fd1 = open("/dev/stdout", O_RDWR);
dup2(fd1, STDOUT_FILENO);
close(fd1);
}
return (-1);
}
if (fd > STDERR_FILENO)
close(fd);
}
The rollback path can still fail because of missing privileges or such,
it's just best effort.
Also, any of the 0,1,2 file descriptors could be associated with other
than stdin/out/err before daemon() is called. However, after returning
with failure, most of the consumers would want to print some sort of
error to stderr and exit afterwards so this approach seems fine to me.
On the other hand, this is different than the behavior of daemon()
elsewhere. Still, preserving compatibility in the error path is probably
too strict. Looking at the above code again, the rollback is probably
too much and its benefit is questionable.
So, in the end I think it could look like this, with a note added to the
man page (that a failure in case noclose is 0 might influence
descriptors 0,1,2):
int fd;
...
if (noclose == 0) {
/*
* Missing the PRIV_FILE_READ privilege may prevent the
* opening of /dev/null to succeed.
*/
if ((fd = open("/dev/null", O_RDWR)) < 0)
return (-1);
/*
* Also, if any of the descriptor redirects fails
* we should return with error to signal to the caller
* that his request cannot be fulfilled properly.
*/
if ((fd != STDIN_FILENO) && dup2(fd, STDIN_FILENO) < 0) {
close(fd);
return (-1);
}
if ((fd != STDOUT_FILENO) && dup2(fd, STDOUT_FILENO) < 0) {
close(fd);
return (-1);
}
if ((fd != STDERR_FILENO) && dup2(fd, STDERR_FILENO) < 0) {
close(fd);
return (-1);
}
if (fd > STDERR_FILENO)
close(fd);
}
Incremental webrev is here (contains merge diffs):
http://cr.opensolaris.org/~vkotal/daemon_libc-4471189.onnv.incr1/
New webrev is here:
http://cr.opensolaris.org/~vkotal/daemon_libc-4471189.onnv.checks/
v.
