depmod: Prevent module dependency files corruption due to parallel invocation.

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 <[email protected]>

---
 tools/depmod.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

tools/depmod.c +5 -1

index 18c0d61b2db3..3b6d16e76160 100644
@@ -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;
 

tools/depmod.c +0 -1

                        fd = openat(dfd, tmp, flags, mode);
                        if (fd < 0) {
                                ERR("openat(%s, %s, %o, %o): %m\n",
--

2.19.2

Quoted 53 lines · Michal Suchanek

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 <[email protected]>

---
 tools/depmod.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

tools/depmod.c +6 -2

index 18c0d61b2db3..3b6d16e76160 100644
@@ -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;
 

tools/depmod.c static int depmod_output(struct depmod *depmod, FILE *out) · line 2429

Quoted 4 lines

tools/depmod.c +3 -1

-                       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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Michal Suchánek

Quoted 54 lines · On Mon, 17 Dec 2018 10:10:37 -0800 Lucas De Marchi <[email protected]>…

Quoted 53 lines · Michal Suchanek

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 <[email protected]>

---
 tools/depmod.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

tools/depmod.c +6 -2

index 18c0d61b2db3..3b6d16e76160 100644
@@ -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;
 

Quoted 7 lines

tools/depmod.c static int depmod_output(struct depmod *depmod, FILE *out) · line 2429

Quoted 4 lines

tools/depmod.c +3 -1

-                       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.

Hi, Michal!

Quoted 66 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Michal Suchánek

Quoted 54 lines · On Mon, 17 Dec 2018 10:10:37 -0800 Lucas De Marchi <[email protected]>…

Quoted 53 lines · Michal Suchanek

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 <[email protected]>

---
 tools/depmod.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

tools/depmod.c +6 -2

index 18c0d61b2db3..3b6d16e76160 100644
@@ -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;
 

Quoted 7 lines

tools/depmod.c static int depmod_output(struct depmod *depmod, FILE *out) · line 2429

Quoted 4 lines

tools/depmod.c +3 -1

-                       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.

Quoted 4 lines

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.
```

Quoted 6 lines

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.

In a couple of places depmod concatenates the module directory and filename with snprintf. This can technically overflow creating an unterminated string if module directory name is long. Use openat instead as is done elsewhere in depmod. This avoids the snprintf, the extra buffer on stack, and the gcc warning. It may even fix a corner case when the module direcotry name is just under PATH_MAX.

Signed-off-by Michal Suchanek <[email protected]>

---
 tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

tools/depmod.c +0 -0

index 3b6d16e76160..fb76f823c016 100644

tools/depmod.c +33 -17

+       int fd, dfd;
+       FILE * ret;
+
+       dfd = open(dname, O_RDONLY);
+       if (dfd < 0) {
+               WRN("could not open directory %s: %m\n", dname);
+               return NULL;
+       }
+
+       fd = openat(dfd, filename, flags);
+       if (fd < 0) {
+               WRN("could not open %s at %s: %m\n", filename, dname);
+               ret = NULL;
+       } else {
+               ret = fdopen(fd, mode);
+               if (!ret)
+                       WRN("could not associate stream with %s: %m\n", filename);
+       }
+       close(dfd);
+       return ret;
+}
+
+
+
 static void depmod_modules_sort(struct depmod *depmod)
 {
-       char order_file[PATH_MAX], line[PATH_MAX];
+       char line[PATH_MAX];
+       const char * order_file = "modules.order";
        FILE *fp;
        unsigned idx = 0, total = 0;
 
-       snprintf(order_file, sizeof(order_file), "%s/modules.order",
-                depmod->cfg->dirname);
-       fp = fopen(order_file, "r");
-       if (fp == NULL) {
-               WRN("could not open %s: %m\n", order_file);
+       fp = dfdopen(depmod->cfg->dirname, order_file, O_RDONLY, "r");
+       if (fp == NULL)
                return;
-       }
 
        while (fgets(line, sizeof(line), fp) != NULL) {
                size_t len = strlen(line);

@@ -1409,8 +1432,8 @@ static void depmod_modules_sort(struct depmod *depmod)
                if (len == 0)
                        continue;
                if (line[len - 1] != '\n') {
-                       ERR("%s:%u corrupted line misses '\\n'\n",
-                               order_file, idx);
+                       ERR("%s/%s:%u corrupted line misses '\\n'\n",
+                               depmod->cfg->dirname, order_file, idx);
                        goto corrupted;
                }
        }

@@ -2287,18 +2310,14 @@ static int output_builtin_bin(struct depmod *depmod, FILE *out)
 {
        FILE *in;
        struct index_node *idx;
-       char infile[PATH_MAX], line[PATH_MAX], modname[PATH_MAX];
+       char line[PATH_MAX], modname[PATH_MAX];
 
        if (out == stdout)
                return 0;
 
-       snprintf(infile, sizeof(infile), "%s/modules.builtin",
-                                                       depmod->cfg->dirname);
-       in = fopen(infile, "r");
-       if (in == NULL) {
-               WRN("could not open %s: %m\n", infile);
+       in = dfdopen(depmod->cfg->dirname, "modules.builtin", O_RDONLY, "r");
+       if (in == NULL)
                return 0;
-       }
 
        idx = index_create();
        if (idx == NULL) {
--

2.19.2