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

Commit Message

Michal Suchánek Dec. 10, 2018, 9:29 p.m. UTC

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 <[e-mail]>
---
 tools/depmod.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Lucas De Marchi Dec. 17, 2018, 6:10 p.m. UTC | #1

On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <[e-mail]> 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 <[e-mail]>
> ---
>  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
>

Michal Suchánek Dec. 17, 2018, 10:07 p.m. UTC | #2

On Mon, 17 Dec 2018 10:10:37 -0800
Lucas De Marchi <[e-mail]> wrote:

> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <[e-mail]> 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 <[e-mail]>
> > ---
> >  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

Yauheni Kaliuta Dec. 18, 2018, 6:47 a.m. UTC | #3

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 <[e-mail]> wrote:

 >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <[e-mail]> 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 <[e-mail]>
 >> > ---
 >> >  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.
```

Michal Suchánek Dec. 18, 2018, 10:26 a.m. UTC | #4

On Tue, 18 Dec 2018 08:47:58 +0200
Yauheni Kaliuta <[e-mail]> 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 <[e-mail]> wrote:  
> 
>  >> On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <[e-mail]> 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 <[e-mail]>
>  >> > ---
>  >> >  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

Patch

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",