Skip to contentSkip to conversation

um: remove uses of variable length arrays

PATCH In review Bartosz Golaszewski 3 participants 6 messages Mar 12, 2019 – Mar 14, 2019 User-mode Linux (UML)

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

PATCH 1/1 um: remove uses of variable length arrays

From: Bartosz Golaszewski <[email protected]>

While the affected code is run in user-mode, the build still warns about it. Convert all uses of VLA to dynamic allocations.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 arch/um/os-Linux/umid.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)
arch/um/os-Linux/umid.c +27 -9
index 998fbb445458..e261656fe9d7 100644
@@ -135,12 +135,18 @@ static int remove_files_and_dir(char *dir)
  */
 static inline int is_umdir_used(char *dir)
 {
-	char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-	char pid[sizeof("nnnnn\0")], *end;
+	char pid[sizeof("nnnnn\0")], *end, *file;
 	int dead, fd, p, n, err;
+	size_t filelen;
 
-	n = snprintf(file, sizeof(file), "%s/pid", dir);
-	if (n >= sizeof(file)) {
+	err = asprintf(&file, "%s/pid", dir);
+	if (err < 0)
+		return 0;
+
+	filelen = strlen(file);
+
+	n = snprintf(file, filelen, "%s/pid", dir);
+	if (n >= filelen) {
 		printk(UM_KERN_ERR "is_umdir_used - pid filename too long\n");
 		err = -E2BIG;
 		goto out;

@@ -185,6 +191,7 @@ static inline int is_umdir_used(char *dir)
 out_close:
 	close(fd);
 out:
+	free(file);
 	return 0;
 }
 

@@ -210,18 +217,21 @@ static int umdir_take_if_dead(char *dir)
 
 static void __init create_pid_file(void)
 {
-	char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-	char pid[sizeof("nnnnn\0")];
+	char pid[sizeof("nnnnn\0")], *file;
 	int fd, n;
 
-	if (umid_file_name("pid", file, sizeof(file)))
+	file = malloc(strlen(uml_dir) + UMID_LEN + sizeof("/pid\0"));
+	if (!file)
 		return;
 
+	if (umid_file_name("pid", file, sizeof(file)))
+		goto out;
+
 	fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644);
 	if (fd < 0) {
 		printk(UM_KERN_ERR "Open of machine pid file \"%s\" failed: "
 		       "%s\n", file, strerror(errno));
-		return;
+		goto out;
 	}
 
 	snprintf(pid, sizeof(pid), "%d\n", getpid());

@@ -231,6 +241,8 @@ static void __init create_pid_file(void)
 		       errno);
 
 	close(fd);
+out:
+	free(file);
 }
 
 int __init set_umid(char *name)

@@ -385,13 +397,19 @@ __uml_setup("uml_dir=", set_uml_dir,
 
 static void remove_umid_dir(void)
 {
-	char dir[strlen(uml_dir) + UMID_LEN + 1], err;
+	char *dir, err;
+
+	dir = malloc(strlen(uml_dir) + UMID_LEN + 1);
+	if (!dir)
+		return;
 
 	sprintf(dir, "%s%s", uml_dir, umid);
 	err = remove_files_and_dir(dir);
 	if (err)
 		os_warn("%s - remove_files_and_dir failed with err = %d\n",
 			__func__, err);
+
+	free(dir);
 }
 
 __uml_exitcall(remove_umid_dir);
Signature · 7 lines
2.20.1


_______________________________________________
linux-um mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-um
maintainer
#1 lore raw
Quoted 92 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

While the affected code is run in user-mode, the build still warns about it. Convert all uses of VLA to dynamic allocations.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
  arch/um/os-Linux/umid.c | 36 +++++++++++++++++++++++++++---------
  1 file changed, 27 insertions(+), 9 deletions(-)
arch/um/os-Linux/umid.c +25 -9
index 998fbb445458..e261656fe9d7 100644
@@ -135,12 +135,18 @@ static int remove_files_and_dir(char *dir)
   */
  static inline int is_umdir_used(char *dir)
  {
-	char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-	char pid[sizeof("nnnnn\0")], *end;
+	char pid[sizeof("nnnnn\0")], *end, *file;
  	int dead, fd, p, n, err;
+	size_t filelen;
 
-	n = snprintf(file, sizeof(file), "%s/pid", dir);
-	if (n >= sizeof(file)) {
+	err = asprintf(&file, "%s/pid", dir);
+	if (err < 0)
+		return 0;
+
+	filelen = strlen(file);
+
+	n = snprintf(file, filelen, "%s/pid", dir);
+	if (n >= filelen) {
  		printk(UM_KERN_ERR "is_umdir_used - pid filename too long\n");
  		err = -E2BIG;
  		goto out;

@@ -185,6 +191,7 @@ static inline int is_umdir_used(char *dir)
  out_close:
  	close(fd);
  out:
+	free(file);
  	return 0;
  }
 

@@ -210,18 +217,21 @@ static int umdir_take_if_dead(char *dir)
 
  static void __init create_pid_file(void)
  {
-	char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-	char pid[sizeof("nnnnn\0")];
+	char pid[sizeof("nnnnn\0")], *file;
  	int fd, n;
 
-	if (umid_file_name("pid", file, sizeof(file)))
+	file = malloc(strlen(uml_dir) + UMID_LEN + sizeof("/pid\0"));
+	if (!file)
  		return;
 
+	if (umid_file_name("pid", file, sizeof(file)))
+		goto out;
+
  	fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644);
  	if (fd < 0) {
  		printk(UM_KERN_ERR "Open of machine pid file \"%s\" failed: "
  		       "%s\n", file, strerror(errno));
-		return;
+		goto out;
  	}
 
  	snprintf(pid, sizeof(pid), "%d\n", getpid());

@@ -231,6 +241,8 @@ static void __init create_pid_file(void)
  		       errno);
 
  	close(fd);
+out:
+	free(file);
  }
 
  int __init set_umid(char *name)

@@ -385,13 +397,19 @@ __uml_setup("uml_dir=", set_uml_dir,
 
  static void remove_umid_dir(void)
  {
-	char dir[strlen(uml_dir) + UMID_LEN + 1], err;
+	char *dir, err;
+
+	dir = malloc(strlen(uml_dir) + UMID_LEN + 1);
+	if (!dir)
+		return;

arch/um/os-Linux/umid.c __uml_setup("uml_dir=", set_uml_dir, · line 415

Quoted 10 lines
arch/um/os-Linux/umid.c +2 -0
  	sprintf(dir, "%s%s", uml_dir, umid);
  	err = remove_files_and_dir(dir);
  	if (err)
  		os_warn("%s - remove_files_and_dir failed with err = %d\n",
  			__func__, err);
+
+	free(dir);
  }
 
  __uml_exitcall(remove_umid_dir);

Thanks for bringing it up. It helped me notice that this is actually broken.

PID can be more than 5 digits nowdays.

Signature · 3 lines
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
author
#2 lore raw

śr., 13 mar 2019 o 10:45 Anton Ivanov
<[email protected]> napisał(a):

Quoted 102 lines · Bartosz Golaszewski
Quoted 100 lines

From: Bartosz Golaszewski <[email protected]>

While the affected code is run in user-mode, the build still warns about it. Convert all uses of VLA to dynamic allocations.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
  arch/um/os-Linux/umid.c | 36 +++++++++++++++++++++++++++---------
  1 file changed, 27 insertions(+), 9 deletions(-)
arch/um/os-Linux/umid.c +27 -9
index 998fbb445458..e261656fe9d7 100644
@@ -135,12 +135,18 @@ static int remove_files_and_dir(char *dir)
   */
  static inline int is_umdir_used(char *dir)
  {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")], *end;
+     char pid[sizeof("nnnnn\0")], *end, *file;
      int dead, fd, p, n, err;
+     size_t filelen;
 
-     n = snprintf(file, sizeof(file), "%s/pid", dir);
-     if (n >= sizeof(file)) {
+     err = asprintf(&file, "%s/pid", dir);
+     if (err < 0)
+             return 0;
+
+     filelen = strlen(file);
+
+     n = snprintf(file, filelen, "%s/pid", dir);
+     if (n >= filelen) {
              printk(UM_KERN_ERR "is_umdir_used - pid filename too long\n");
              err = -E2BIG;
              goto out;

@@ -185,6 +191,7 @@ static inline int is_umdir_used(char *dir)
  out_close:
      close(fd);
  out:
+     free(file);
      return 0;
  }
 

@@ -210,18 +217,21 @@ static int umdir_take_if_dead(char *dir)
 
  static void __init create_pid_file(void)
  {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")];
+     char pid[sizeof("nnnnn\0")], *file;
      int fd, n;
 
-     if (umid_file_name("pid", file, sizeof(file)))
+     file = malloc(strlen(uml_dir) + UMID_LEN + sizeof("/pid\0"));
+     if (!file)
              return;
 
+     if (umid_file_name("pid", file, sizeof(file)))
+             goto out;
+
      fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644);
      if (fd < 0) {
              printk(UM_KERN_ERR "Open of machine pid file \"%s\" failed: "
                     "%s\n", file, strerror(errno));
-             return;
+             goto out;
      }
 
      snprintf(pid, sizeof(pid), "%d\n", getpid());

@@ -231,6 +241,8 @@ static void __init create_pid_file(void)
                     errno);
 
      close(fd);
+out:
+     free(file);
  }
 
  int __init set_umid(char *name)

@@ -385,13 +397,19 @@ __uml_setup("uml_dir=", set_uml_dir,
 
  static void remove_umid_dir(void)
  {
-     char dir[strlen(uml_dir) + UMID_LEN + 1], err;
+     char *dir, err;
+
+     dir = malloc(strlen(uml_dir) + UMID_LEN + 1);
+     if (!dir)
+             return;
 
      sprintf(dir, "%s%s", uml_dir, umid);
      err = remove_files_and_dir(dir);
      if (err)
              os_warn("%s - remove_files_and_dir failed with err = %d\n",
                      __func__, err);
+
+     free(dir);
  }
Quoted 7 lines

arch/um/os-Linux/umid.c __uml_setup("uml_dir=", set_uml_dir, · line 415

Quoted 2 lines
arch/um/os-Linux/umid.c +0 -0
  __uml_exitcall(remove_umid_dir);

Thanks for bringing it up. It helped me notice that this is actually broken.

PID can be more than 5 digits nowdays.

Do we have a macro in the kernel that yields the minimum size of an array to fit a number? I can't find it. I vagely remember seeing something like this in systemd tree. If it doesn't exist in the kernel then maybe we should copy it over.

Bart

#3 lore raw

Hi Bartosz,

Quoted 8 lines · Bartosz Golaszewski

śr., 13 mar 2019 o 10:45 Anton Ivanov
<[email protected]> napisał(a):

Quoted 1 line

PID can be more than 5 digits nowdays.

Do we have a macro in the kernel that yields the minimum size of an array to fit a number? I can't find it. I vagely remember seeing something like this in systemd tree. If it doesn't exist in the kernel then maybe we should copy it over.

1 + const_ilog2(n) / log2(10) = 1 + const_ilog2(n) / 3.321928

Using fixed point calculations, that can be approximated as

    1 + 19728 * const_ilog2(n) >> 16
    1 + 77 * const_ilog2(n) >> 8

(the factors might be one off)

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
author
#4 lore raw
In reply to Anton Ivanov

śr., 13 mar 2019 o 10:45 Anton Ivanov
<[email protected]> napisał(a):

Quoted 102 lines · Bartosz Golaszewski
Quoted 100 lines

From: Bartosz Golaszewski <[email protected]>

While the affected code is run in user-mode, the build still warns about it. Convert all uses of VLA to dynamic allocations.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
  arch/um/os-Linux/umid.c | 36 +++++++++++++++++++++++++++---------
  1 file changed, 27 insertions(+), 9 deletions(-)
arch/um/os-Linux/umid.c +27 -9
index 998fbb445458..e261656fe9d7 100644
@@ -135,12 +135,18 @@ static int remove_files_and_dir(char *dir)
   */
  static inline int is_umdir_used(char *dir)
  {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")], *end;
+     char pid[sizeof("nnnnn\0")], *end, *file;
      int dead, fd, p, n, err;
+     size_t filelen;
 
-     n = snprintf(file, sizeof(file), "%s/pid", dir);
-     if (n >= sizeof(file)) {
+     err = asprintf(&file, "%s/pid", dir);
+     if (err < 0)
+             return 0;
+
+     filelen = strlen(file);
+
+     n = snprintf(file, filelen, "%s/pid", dir);
+     if (n >= filelen) {
              printk(UM_KERN_ERR "is_umdir_used - pid filename too long\n");
              err = -E2BIG;
              goto out;

@@ -185,6 +191,7 @@ static inline int is_umdir_used(char *dir)
  out_close:
      close(fd);
  out:
+     free(file);
      return 0;
  }
 

@@ -210,18 +217,21 @@ static int umdir_take_if_dead(char *dir)
 
  static void __init create_pid_file(void)
  {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")];
+     char pid[sizeof("nnnnn\0")], *file;
      int fd, n;
 
-     if (umid_file_name("pid", file, sizeof(file)))
+     file = malloc(strlen(uml_dir) + UMID_LEN + sizeof("/pid\0"));
+     if (!file)
              return;
 
+     if (umid_file_name("pid", file, sizeof(file)))
+             goto out;
+
      fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644);
      if (fd < 0) {
              printk(UM_KERN_ERR "Open of machine pid file \"%s\" failed: "
                     "%s\n", file, strerror(errno));
-             return;
+             goto out;
      }
 
      snprintf(pid, sizeof(pid), "%d\n", getpid());

@@ -231,6 +241,8 @@ static void __init create_pid_file(void)
                     errno);
 
      close(fd);
+out:
+     free(file);
  }
 
  int __init set_umid(char *name)

@@ -385,13 +397,19 @@ __uml_setup("uml_dir=", set_uml_dir,
 
  static void remove_umid_dir(void)
  {
-     char dir[strlen(uml_dir) + UMID_LEN + 1], err;
+     char *dir, err;
+
+     dir = malloc(strlen(uml_dir) + UMID_LEN + 1);
+     if (!dir)
+             return;
 
      sprintf(dir, "%s%s", uml_dir, umid);
      err = remove_files_and_dir(dir);
      if (err)
              os_warn("%s - remove_files_and_dir failed with err = %d\n",
                      __func__, err);
+
+     free(dir);
  }
Quoted 8 lines

arch/um/os-Linux/umid.c __uml_setup("uml_dir=", set_uml_dir, · line 415

Quoted 2 lines
arch/um/os-Linux/umid.c +0 -0
  __uml_exitcall(remove_umid_dir);

Thanks for bringing it up. It helped me notice that this is actually broken.

PID can be more than 5 digits nowdays.

--

Do you want to take this patch anyway and then apply the fix for the array on top of that or do you prefer it be fixed before that?

Bart

maintainer
#5 lore raw
Quoted 107 lines · Bartosz Golaszewski

śr., 13 mar 2019 o 10:45 Anton Ivanov
<[email protected]> napisał(a):

Quoted 105 lines · Bartosz Golaszewski
Quoted 92 lines

From: Bartosz Golaszewski <[email protected]>

While the affected code is run in user-mode, the build still warns about it. Convert all uses of VLA to dynamic allocations.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
   arch/um/os-Linux/umid.c | 36 +++++++++++++++++++++++++++---------
   1 file changed, 27 insertions(+), 9 deletions(-)
arch/um/os-Linux/umid.c +25 -9
index 998fbb445458..e261656fe9d7 100644
@@ -135,12 +135,18 @@ static int remove_files_and_dir(char *dir)
    */
   static inline int is_umdir_used(char *dir)
   {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")], *end;
+     char pid[sizeof("nnnnn\0")], *end, *file;
       int dead, fd, p, n, err;
+     size_t filelen;
 
-     n = snprintf(file, sizeof(file), "%s/pid", dir);
-     if (n >= sizeof(file)) {
+     err = asprintf(&file, "%s/pid", dir);
+     if (err < 0)
+             return 0;
+
+     filelen = strlen(file);
+
+     n = snprintf(file, filelen, "%s/pid", dir);
+     if (n >= filelen) {
               printk(UM_KERN_ERR "is_umdir_used - pid filename too long\n");
               err = -E2BIG;
               goto out;

@@ -185,6 +191,7 @@ static inline int is_umdir_used(char *dir)
   out_close:
       close(fd);
   out:
+     free(file);
       return 0;
   }
 

@@ -210,18 +217,21 @@ static int umdir_take_if_dead(char *dir)
 
   static void __init create_pid_file(void)
   {
-     char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")];
-     char pid[sizeof("nnnnn\0")];
+     char pid[sizeof("nnnnn\0")], *file;
       int fd, n;
 
-     if (umid_file_name("pid", file, sizeof(file)))
+     file = malloc(strlen(uml_dir) + UMID_LEN + sizeof("/pid\0"));
+     if (!file)
               return;
 
+     if (umid_file_name("pid", file, sizeof(file)))
+             goto out;
+
       fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644);
       if (fd < 0) {
               printk(UM_KERN_ERR "Open of machine pid file \"%s\" failed: "
                      "%s\n", file, strerror(errno));
-             return;
+             goto out;
       }
 
       snprintf(pid, sizeof(pid), "%d\n", getpid());

@@ -231,6 +241,8 @@ static void __init create_pid_file(void)
                      errno);
 
       close(fd);
+out:
+     free(file);
   }
 
   int __init set_umid(char *name)

@@ -385,13 +397,19 @@ __uml_setup("uml_dir=", set_uml_dir,
 
   static void remove_umid_dir(void)
   {
-     char dir[strlen(uml_dir) + UMID_LEN + 1], err;
+     char *dir, err;
+
+     dir = malloc(strlen(uml_dir) + UMID_LEN + 1);
+     if (!dir)
+             return;

arch/um/os-Linux/umid.c __uml_setup("uml_dir=", set_uml_dir, · line 415

Quoted 10 lines
arch/um/os-Linux/umid.c +2 -0
       sprintf(dir, "%s%s", uml_dir, umid);
       err = remove_files_and_dir(dir);
       if (err)
               os_warn("%s - remove_files_and_dir failed with err = %d\n",
                       __func__, err);
+
+     free(dir);
   }
 
   __uml_exitcall(remove_umid_dir);

Thanks for bringing it up. It helped me notice that this is actually broken.

Quoted 8 lines
Quoted 3 lines

PID can be more than 5 digits nowdays.

--

Do you want to take this patch anyway and then apply the fix for the array on top of that or do you prefer it be fixed before that?

Bart

I am OK to take it as is and have the PID length fixed after that.

Signature · 3 lines
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/