[PATCH] gensignkey: ensure host keys are flushed to disk

[Peter Korsgaard]

>>>>> "DELOGET," == DELOGET, Emmanuel <emmanuel.deloget at sfr.com> writes:

Hi (this time from an address subscribed to the list),

 >> out:
 >> if (fd >= 0) {
 >> +		fsync(fd);

 > Instead of fsync, I think it sould be better to open the file with
 > O_SYNC on the systems that allows it (and add a fsync fallback on the
 > systems where O_SYNC is not available). While it will degrade the
 > performance a bit (since the file is quite small, I guess that adding
 > O_SYNC would not be that bad), it will ensure that the write will not
 > return until the data + metadata is present on disk - meaning that we'll
 > avoid race conditions that could occur between the write and the fsync
 > operation.

This is the write to the temporary file, so it doesn't really
matter. The only important thing is to ensure the data is flushed before
the link / dir fsync is done.


 >> +#ifdef HAVE_LIBGEN_H
 >> +	/* ensure directory update is flushed to disk */
 >> +	fn_dir = strdup(fn);
 >> +	if (fn_dir != NULL) {

 > if (fn_dir) ? Checking for NULL is not necessar - the C standard says
 > that we have a known path to promote pointers to boolean (

Sure they are equivalent, just trying to follow the code style of the
rest of dropbear.


 > Anyway, it sound weird to not do a check if a malloc failed (which is
 > what is tested here) - thus I see 3 possibilities:

 > * stick with this solution
 > * use a PATH_MAX on-stack variable to store a copy of fn
 > * return an error if strdup() (or malloc) fails. 

 > I'm not sure there is one good answer to this specific point.

I'm not sure what you mean here? Are you referring to the fact that I
skip the fsync if the strdup fails?

I think the current behaviour is the most sensible. Strdup is very
unlikely to fail, and we don't want to quit the host key writing even if
it did (as you can then not use dropbear at all).


 >> +		char *dir = dirname(fn_dir);
 >> +		int dirfd = open(dir, O_RDONLY);

 > The O_DIRECTORY flag is missing (at least on Linux) to make sure that
 > the pathname is indeed a directory (it /might/ be a file).

Ok, but even so the fsync would be harmless.


 >> +
 >> +		if (dirfd != -1) {
 >> +			fsync(dirfd);

 > O_RDONLY file descriptors can be fsysnc only on Linux (that's a well
 > documented POSIX conformance issue). We should make sure that the fd is
 > writable before we fsync it. 

Ok, out of interest - What systems disallow fsync on RO directory FDs?
Changing to O_RDWR should be fine as we're already writing to the
directory.

Thanks for the review!

-- 
Bye, Peter Korsgaard