On Thu, Feb 16, 2006 at 01:22:20AM +1300, Matt Brown wrote: > Hi Javier,
Hi there. I hope you don't mind me being a little bit picky, but I think it
helps you hone your skills :-)
>
> > * there's a buffer overflow if 'fname' is longer than 512 chars. buf should
> > *not* be of a static size
>
> There is no buffer overflow, the worst would be the pidfile name is
> truncated, we are using snprintf not sprintf. However I have added
> further error checking to handle the (unlikely) case of the pidfile name
> exceeding 512 characters.
You are right, I saw a static buffer and read sprintf instead of snprintf.
That should be safe.
> > * it should refuse to run (or create a pidfile) if the pidfile already
> > exists.
> <snip>
> Agreed - improved pidfile handling is now in the patch.
Which is not done properly, as it contains a race condition. You should,
instead of checking for the file just try this:
fd = open(&buf[0], O_WRONLY|O_CREAT|O_TRUNC|O_EXCL, 0660)
if ( fd < 0 ) {
/* Error creating file, check out why, EEXIST? */
(...)
return -1;
}
> > I suggest you reuse a pidfile creation function from some other tested
> > program, there should be plenty of examples around.
>
> Unfortunately most are heavily integrated with the specified application
> and its error logging routines etc. This is why we originally developed
> the put_pid function that I included in the original patch, it acts as a
> library function within my group of friends. I've now updated and
> improved it based on your (much appreciated) comments on its
> deficiencies.
I'm pretty sure there are libraries that do implement that function already.
For exmaple, the 'daemon' package in Debian, which is based on libslack (see
http://libslack.org/) already provides a number of library functions for
daemons, such as daemon_pidfile (see
http://libslack.org/manpages/daemon.3.html). Most of the code you want
is already there (see daemon_construct_pidfile() and
daemon_lock_pidfile() or daemon_pidfile_unlocked)
> > I don't like the patch to portreserve.c and I object to this NMU. You *can*
> > NMU, however, to include the changes to the init.d script.
>
> Are you happier with this revised patch?
Ummm. No, I'm not happy with it because:
* the race condition in do_it()
* the prinf() messages should go to STDERR and print sterror(errno)
* it doesn't make sense to use the same function to both create and remove
the pidfile. I.e. why call do_it() it with a parameter instead of just having
a function that does:
int remove_pidfile(const char *pidfile) {
if (pidfile) {
if ( ! unlink(pidfile) ) {
fprintf (STDERR, "Could not remove pidfile %s: %s\n",
pidfile, strerror(errno));
return -1;
}
}
return 0;
}
?
The use of the do_it() function for everything leads to confusion.
Would you mind looking up how daemon() does this and maybe implement a
similar solution?
Regards
Javier
signature.asc
Description: Digital signature
