On Tue, 18 Dec 2018 08:47:58 +0200 Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx> wrote: > Hi, Michal! > > >>>>> On Mon, 17 Dec 2018 23:07:43 +0100, Michal Suchánek wrote: > > > On Mon, 17 Dec 2018 10:10:37 -0800 > > Lucas De Marchi <lucas.de.marchi@xxxxxxxxx> wrote: > > >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@xxxxxxx> wrote: > >> > > >> > Depmod does not use unique filename for temporary files. There is no > >> > guarantee the user does not attempt to run mutiple depmod processes in > >> > parallel. If that happens a temporary file might be created by > >> > depmod(1st), truncated by depmod(2nd), and renamed to final name by > >> > depmod(1st) resulting in corrupted file seen by user. > >> > > >> > Due to missing mkstempat() this is more complex than it should be. > >> > Adding PID and timestamp to the filename should be reasonably reliable. > >> > Adding O_EXCL as mkstemp does fails creating the file rather than > >> > corrupting existing file. > >> > > >> > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx> > >> > --- > >> > tools/depmod.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/tools/depmod.c b/tools/depmod.c > >> > index 18c0d61b2db3..3b6d16e76160 100644 > >> > --- a/tools/depmod.c > >> > +++ b/tools/depmod.c > >> > @@ -29,6 +29,7 @@ > >> > #include <string.h> > >> > #include <unistd.h> > >> > #include <sys/stat.h> > >> > +#include <sys/time.h> > >> > #include <sys/utsname.h> > >> > > >> > #include <shared/array.h> > >> > @@ -2398,6 +2399,9 @@ static int depmod_output(struct depmod *depmod, FILE *out) > >> > }; > >> > const char *dname = depmod->cfg->dirname; > >> > int dfd, err = 0; > >> > + struct timeval tv; > >> > + > >> > + gettimeofday(&tv, NULL); > >> > > >> > if (out != NULL) > >> > dfd = -1; > >> > @@ -2412,15 +2416,17 @@ static int depmod_output(struct depmod *depmod, FILE *out) > >> > > >> > for (itr = depfiles; itr->name != NULL; itr++) { > >> > FILE *fp = out; > >> > - char tmp[NAME_MAX] = ""; > >> > + char tmp[NAME_MAX + 1] = ""; > >> > int r, ferr; > >> > > >> > if (fp == NULL) { > >> > - int flags = O_CREAT | O_TRUNC | O_WRONLY; > >> > + int flags = O_CREAT | O_TRUNC | O_WRONLY | O_EXCL; > >> > int mode = 0644; > >> > int fd; > >> > > >> > - snprintf(tmp, sizeof(tmp), "%s.tmp", itr->name); > >> > + snprintf(tmp, sizeof(tmp), "%s.%i.%li.%li", itr->name, getpid(), > >> > + tv.tv_usec, tv.tv_sec); > >> > + tmp[NAME_MAX] = 0; > >> > >> why? snprintf is guaranteed to nul-terminate the string. > >> > > AFAIK it is guaranteed to not write after end of buffer. It is > > not guaranteed to terminate the string. To guarantee > > terminated string you need large enough buffer or a nul after > > the buffer. Or asprintf. > > Actually, it is. VS strncpy(). May be it is not so clear from the > man page: > > ``` > The functions snprintf() and vsnprintf() write at most size > bytes (including the terminating null byte ('\0')) to str. > ``` > Yes, that's it. The POSIX page is much clearer: >> The snprintf() function shall be equivalent to sprintf(), with the >> addition of the n argument which states the size of the buffer referred >> to by s. If n is zero, nothing shall be written and s may be a null >> pointer. Otherwise, output bytes beyond the n‐1st shall be >> discarded instead of being written to the array, and a null byte is >> written at the end of the bytes actually written into the array. Thanks Michal