From: Matthias Lederhofer <matled@gmx.net>
Date: Thu, 13 Jul 2006 14:04:30 GMT
Subject: Re: [PATCH 2/5] daemon: if one of the standard fds is missing open it to /dev/null
Message-ID: <E1G11nq-00076g-Aa@moooo.ath.cx>
In-Reply-To: <20060713152725.7a5081df.froese@gmx.de>

Edgar Toernig <froese@gmx.de> wrote:
> Matthias Lederhofer wrote:
> >
> > +/* if any standard file descriptor is missing open it to /dev/null */
> > +static void sanitize_stdfds(void)
> > +{
> > +	int devnull = -1, i;
> > +	struct stat buf;
> > +	for (i = 0; i < 3; ++i) {
> > +		if (fstat(i, &buf) != -1)
> > +			continue;
> > +		if (devnull == -1 &&
> > +			(devnull = open("/dev/null", O_RDWR, 0)) == -1)
> > +			die("open /dev/null failed: %s", strerror(errno));
> > +		if (dup2(devnull, i) != i)
> > +			die("dup2 failed: %s", strerror(errno));
> > +	}
> > +	if (devnull != -1)
> > +		close(devnull);
> > +}
> 
> This looks broken.  The open will return i as this is
> the lowest free fd.  I don't know what POSIX says
> about dup2(i,i) but anyway, you close it at the end
> which completely defeats the intent of the function.
> 
> How's this?
> 
> 	devnull = open("/dev/null", O_RDWR, 0);
> 	if (devnull == 0)
> 		devnull = dup(devnull);
> 	if (devnull == 1)
> 		devnull = dup(devnull);
> 	if (devnull == -1)
> 		die("open/dup /dev/null failed: %s", strerror(errno));
> 	if (devnull > 2)
> 		close(devnull);

You're right (also for the daemonize function to use sanitize_stdfds).
The code looks good to me, this could also be done using a while-loop
(making it a little bit shorter, I don't know what is easier to read):

    devnull = open("/dev/null", O_RDWR, 0);
    while (devnull != -1 && devnull < 2)
        dup(devnull);
    if (devnull == -1)
        die("..");
    close(devnull);

(This is similar to what Andre Noll posted.)

I'll correct and resend those patches later.