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

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

[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 random number to the filename should be reasonably
reliable. Adding O_EXCL as mkstemp does fails creating the file rather
than corrupting existing file.

Also prevent dependency files missing. This happens because target files
are removed before renaming the temporary file.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 tools/depmod.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 989d9077926c..5526ac892cf8 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,10 @@ 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);
+	srand(tv.tv_sec);
 
 	if (out != NULL)
 		dfd = -1;
@@ -2412,15 +2417,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.%i", itr->name, getpid(),
+					rand());
+			tmp[NAME_MAX] = 0;
 			fd = openat(dfd, tmp, flags, mode);
 			if (fd < 0) {
 				ERR("openat(%s, %s, %o, %o): %m\n",
@@ -2451,7 +2458,6 @@ static int depmod_output(struct depmod *depmod, FILE *out)
 			break;
 		}
 
-		unlinkat(dfd, itr->name, 0);
 		if (renameat(dfd, tmp, dfd, itr->name) != 0) {
 			err = -errno;
 			CRIT("renameat(%s, %s, %s, %s): %m\n",
-- 
2.19.2

[PATCH v2 0/3] Fix dependency file corruption with parallel depmod invocation

[Michal Suchanek]

The files produced by depmod may be corrupted if multiple depmod processes
running in parallel try to update them. Since there is nothing stopping the
user running depmod in parallel this looks like a bug in depmod.

When a package manager is used it synchronizes adding or removeing module files
and depmod invocation. However, user might use other script that is not
synchronized with package manager or invoke depmod by hand.

When global synchronizarion is missing we cannot guarantee completely
consistent state because adding and removing module files is anynchronous.
However, we should guarantee the system is bootable if the user did not remove
the modules needed for boot. For that the dependency file should not get
removed or corrupted.

Thanks

Michal

Michal Suchanek (3):
  depmod: prevent module dependency files missing during depmod
    invocation.
  depmod: prevent module dependency files corruption due to parallel
    invocation.
  depmod: shut up gcc insufficinet buffer warning.

 tools/depmod.c | 64 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 20 deletions(-)

-- 
2.19.2

[PATCH v2 1/3] depmod: prevent module dependency files missing during depmod invocation.

[Michal Suchanek]

Depmod deletes the module dependency files before moving the temporary
files in their place. This results in user seeing no dependency files
while they are updated. Remove the unlink call. The rename call should
suffice to move the new file in place and unlink the old one. It should
also do both atomically so there is no window when no dependency file
exists.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 tools/depmod.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 989d9077926c..18c0d61b2db3 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2451,7 +2451,6 @@ static int depmod_output(struct depmod *depmod, FILE *out)
 			break;
 		}
 
-		unlinkat(dfd, itr->name, 0);
 		if (renameat(dfd, tmp, dfd, itr->name) != 0) {
 			err = -errno;
 			CRIT("renameat(%s, %s, %s, %s): %m\n",
-- 
2.19.2

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

[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 <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;
 			fd = openat(dfd, tmp, flags, mode);
 			if (fd < 0) {
 				ERR("openat(%s, %s, %o, %o): %m\n",
-- 
2.19.2

[PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning.

[Michal Suchanek]

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 <msuchanek@suse.de>
---
 tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 3b6d16e76160..fb76f823c016 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod)
 	return 0;
 }
 
+static FILE * dfdopen(const char * dname, const char * filename, int flags, const char * mode)
+	{
+	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

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

[Lucas De Marchi]

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
>


-- 
Lucas De Marchi

Re: [PATCH v2 3/3] depmod: shut up gcc insufficinet buffer warning.

[Lucas De Marchi]

On Mon, Dec 10, 2018 at 2:03 PM Michal Suchanek <msuchanek@suse.de> wrote:
>
> 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 <msuchanek@suse.de>
> ---
>  tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/tools/depmod.c b/tools/depmod.c
> index 3b6d16e76160..fb76f823c016 100644
> --- a/tools/depmod.c
> +++ b/tools/depmod.c
> @@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod)
>         return 0;
>  }
>
> +static FILE * dfdopen(const char * dname, const char * filename, int flags, const char * mode)

coding style here is not consistent with the rest. We don't have space
after "*".

> +       {

and this is wrongly indented.

Other than that this LGTM and I can fixup the styling issues while applying.

Thanks.
Lucas De Marchi

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


-- 
Lucas De Marchi

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

[Michal Suchánek]

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

[PATCH v3 0/3] Fix dependency file corruption with parallel depmod invocation

[Michal Suchanek]

The files produced by depmod may be corrupted if multiple depmod processes
running in parallel try to update them. Since there is nothing stopping the
user running depmod in parallel this looks like a bug in depmod.

v3:
 - remove superfluous whitespace, open flag, and duplicate nul termination

v2:
 - split into separate patches, add gcc warning fix

Michal Suchanek (3):
  depmod: prevent module dependency files missing during depmod
    invocation.
  depmod: prevent module dependency files corruption due to parallel
    invocation.
  depmod: shut up gcc insufficinet buffer warning.

 tools/depmod.c | 61 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 19 deletions(-)

-- 
2.19.2

[PATCH v3 1/3] depmod: prevent module dependency files missing during depmod invocation.

[Michal Suchanek]

Depmod deletes the module dependency files before moving the temporary
files in their place. This results in user seeing no dependency files
while they are updated. Remove the unlink call. The rename call should
suffice to move the new file in place and unlink the old one. It should
also do both atomically so there is no window when no dependency file
exists.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 tools/depmod.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 989d9077926c..18c0d61b2db3 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2451,7 +2451,6 @@ static int depmod_output(struct depmod *depmod, FILE *out)
 			break;
 		}
 
-		unlinkat(dfd, itr->name, 0);
 		if (renameat(dfd, tmp, dfd, itr->name) != 0) {
 			err = -errno;
 			CRIT("renameat(%s, %s, %s, %s): %m\n",
-- 
2.19.2

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

[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 <msuchanek@suse.de>
---
v3:
 - remove superfluous code to terminate the string - snprintf should do
 it
 - remove superfluous O_TRUNC - not needed with O_EXCL
---
 tools/depmod.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 18c0d61b2db3..0f7e33ccfd59 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;
@@ -2416,11 +2420,12 @@ static int depmod_output(struct depmod *depmod, FILE *out)
 		int r, ferr;
 
 		if (fp == NULL) {
-			int flags = O_CREAT | O_TRUNC | O_WRONLY;
+			int flags = O_CREAT | O_EXCL | O_WRONLY;
 			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);
 			fd = openat(dfd, tmp, flags, mode);
 			if (fd < 0) {
 				ERR("openat(%s, %s, %o, %o): %m\n",
-- 
2.19.2

[PATCH v3 3/3] depmod: shut up gcc insufficinet buffer warning.

[Michal Suchanek]

In a couple of places depmod concatenates the module directory and
filename with snprintf. This can technically overflow creating wrong
path 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.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
v3:
 - remove superfluous whitespace
---
 tools/depmod.c | 51 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 0f7e33ccfd59..0cf845a0dbc7 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1389,19 +1389,42 @@ static int depmod_modules_build_array(struct depmod *depmod)
 	return 0;
 }
 
+static FILE *dfdopen(const char *dname, const char *filename, int flags, const char *mode)
+{
+	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

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

[Yauheni Kaliuta]

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

-- 
WBR,
Yauheni Kaliuta

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

[Michal Suchánek]

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