| Message ID | 636a25c106ac3da29803025248a237e9d23f4e9d.1544476531.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix dependency file corruption with parallel depmod invocation | expand |
On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> 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@suse.de> > --- > 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. Lucas De Marchi > fd = openat(dfd, tmp, flags, mode); > if (fd < 0) { > ERR("openat(%s, %s, %o, %o): %m\n", > -- > 2.19.2 >
On Mon, 17 Dec 2018 10:10:37 -0800 Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> 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@suse.de> > > --- > > 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. Thanks Michal
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@gmail.com> wrote: >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> 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@suse.de> >> > --- >> > 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. ```
On Tue, 18 Dec 2018 08:47:58 +0200 Yauheni Kaliuta <yauheni.kaliuta@redhat.com> 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@gmail.com> wrote: > > >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> 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@suse.de> > >> > --- > >> > 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
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; fd = openat(dfd, tmp, flags, mode); if (fd < 0) { ERR("openat(%s, %s, %o, %o): %m\n",
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@suse.de> --- tools/depmod.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)