Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation.

["Michal Suchánek" <msuchanek@suse.de>]

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