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

From: "Michal Suchánek" <msuchanek@suse.de>
To: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Cc: Lucas De Marchi <lucas.de.marchi@gmail.com>,
	linux-modules <linux-modules@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] depmod: prevent module dependency files corruption due to parallel invocation.
Date: Tue, 18 Dec 2018 11:26:14 +0100	[thread overview]
Message-ID: <20181218112614.07242b2e@naga> (raw)
In-Reply-To: <xunyd0pzl49t.fsf@redhat.com>

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