From: Paul Smith <psmith@gnu.org>
Date: Mon, 31 Oct 2022 05:48:33 +0000 (-0400)
Subject: Fix issues found by ASAN and Coverity
X-Git-Tag: 4.4~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=38b19976f50af0c898030adcb86320bdfe52a159;p=thirdparty%2Fmake.git

Fix issues found by ASAN and Coverity

* tests/test_driver.pl: Preserve the LSAN_OPTIONS variable.
* tests/scripts/targets/ONESHELL: Don't set a local variable.
* tests/scripts/functions/let: Test empty let variable.
* src/posixos.c (osync_parse_mutex): Free existing osync_tmpfile.
* src/misc.c (get_tmpfd): Set umask() before invoking mkstemp().
* src/ar.c (ar_parse_name): Check invalid name (shouldn't happen).
* src/function.c (define_new_function): Free previous function entry
when replacing it with a new one.
* src/job.c (child_execute_job): Initialize pid for safety.
(construct_command_argv_internal): In oneshell mode ensure that the
returned argv has the right format (0th element is a pointer to the
entire buffer).
---

diff --git a/src/ar.c b/src/ar.c
index 8b02f047..7b607dcd 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -36,7 +36,7 @@ ar_name (const char *name)
   const char *p = strchr (name, '(');
   const char *end;
 
-  if (p == 0 || p == name)
+  if (p == NULL || p == name)
     return 0;
 
   end = p + strlen (p) - 1;
@@ -61,6 +61,9 @@ ar_parse_name (const char *name, char **arname_p, char **memname_p)
 
   *arname_p = xstrdup (name);
   p = strchr (*arname_p, '(');
+  /* This is never called unless ar_name() is true so p cannot be NULL.  */
+  if (!p)
+    OS (fatal, NILF, "Internal: ar_parse_name: bad name '%s'", *arname_p);
   *(p++) = '\0';
   p[strlen (p) - 1] = '\0';
   *memname_p = p;
diff --git a/src/dir.c b/src/dir.c
index 3577a12d..b47e94fe 100644
--- a/src/dir.c
+++ b/src/dir.c
@@ -601,7 +601,7 @@ find_directory (const char *name)
   /* Point the name-hashed entry for DIR at its contents data.  */
   dir->contents = dc;
 
-  /* If the contents have changed, we need to reseet.  */
+  /* If the contents have changed, we need to reseed.  */
   if (dc->counter != command_count)
     {
       if (dc->counter)
diff --git a/src/function.c b/src/function.c
index db5e1b51..f0ef3434 100644
--- a/src/function.c
+++ b/src/function.c
@@ -2811,7 +2811,8 @@ define_new_function (const floc *flocp, const char *name,
   ent->adds_command = 1;
   ent->fptr.alloc_func_ptr = func;
 
-  hash_insert (&function_table, ent);
+  ent = hash_insert (&function_table, ent);
+  free (ent);
 }
 
 void
diff --git a/src/job.c b/src/job.c
index ed951026..b78f279c 100644
--- a/src/job.c
+++ b/src/job.c
@@ -2271,7 +2271,7 @@ child_execute_job (struct childbase *child, int good_stdin, char **argv)
   const int fdin = good_stdin ? FD_STDIN : get_bad_stdin ();
   int fdout = FD_STDOUT;
   int fderr = FD_STDERR;
-  pid_t pid;
+  pid_t pid = -1;
   int r;
 #if defined(USE_POSIX_SPAWN)
   char *cmd;
@@ -3364,30 +3364,44 @@ construct_command_argv_internal (char *line, char **restp, const char *shell,
 #endif /* WINDOWS32 */
         /* Create an argv list for the shell command line.  */
         {
-          int n = 0;
+          int n = 1;
+          char *nextp;
 
           new_argv = xmalloc ((4 + sflags_len/2) * sizeof (char *));
-          new_argv[n++] = xstrdup (shell);
+
+          nextp = new_argv[0] = xmalloc (shell_len + sflags_len + line_len + 3);
+          nextp = mempcpy (nextp, shell, shell_len + 1);
 
           /* Chop up the shellflags (if any) and assign them.  */
           if (! shellflags)
-            new_argv[n++] = xstrdup ("");
+            {
+              new_argv[n++] = nextp;
+              *(nextp++) = '\0';
+            }
           else
             {
               /* Parse shellflags using construct_command_argv_internal to
                  handle quotes. */
-              char **argv, **a;
-              char *f;
-              f = alloca (sflags_len + 1); /* +1 for null terminator.  */
+              char **argv;
+              char *f = alloca (sflags_len + 1);
               memcpy (f, shellflags, sflags_len + 1);
               argv = construct_command_argv_internal (f, 0, 0, 0, 0, flags, 0);
-              for (a = argv; a && *a; ++a)
-                new_argv[n++] = *a;
-              free (argv);
+              if (argv)
+                {
+                  char **a;
+                  for (a = argv; *a; ++a)
+                    {
+                      new_argv[n++] = nextp;
+                      nextp = stpcpy (nextp, *a) + 1;
+                    }
+                  free (argv[0]);
+                  free (argv);
+                }
             }
 
           /* Set the command to invoke.  */
-          new_argv[n++] = line;
+          new_argv[n++] = nextp;
+          memcpy(nextp, line, line_len + 1);
           new_argv[n++] = NULL;
         }
         return new_argv;
diff --git a/src/misc.c b/src/misc.c
index bf6fe7ca..8264fe9f 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -668,6 +668,7 @@ get_tmpfd (char **name)
 {
   int fd = -1;
   char *tmpnm;
+  mode_t mask;
 
   /* If there's an os-specific way to get an anoymous temp file use it.  */
   if (!name)
@@ -677,6 +678,10 @@ get_tmpfd (char **name)
         return fd;
     }
 
+  /* Preserve the current umask, and set a restrictive one for temp files.
+     Only really needed for mkstemp() but won't hurt for the open method.  */
+  mask = umask (0077);
+
 #if defined(HAVE_MKSTEMP)
   tmpnm = get_tmptemplate ();
 
@@ -704,6 +709,8 @@ get_tmpfd (char **name)
       free (tmpnm);
     }
 
+  umask (mask);
+
   return fd;
 }
 
diff --git a/src/posixos.c b/src/posixos.c
index 63201b40..44aeb346 100644
--- a/src/posixos.c
+++ b/src/posixos.c
@@ -673,6 +673,7 @@ osync_parse_mutex (const char *mutex)
       return 0;
     }
 
+  free (osync_tmpfile);
   osync_tmpfile = xstrdup (mutex + CSTRLEN (MUTEX_PREFIX));
 
   EINTRLOOP (osync_handle, open (osync_tmpfile, O_WRONLY));
diff --git a/tests/scripts/features/varnesting b/tests/scripts/features/varnesting
index d8f3ffbb..0fbb332d 100644
--- a/tests/scripts/features/varnesting
+++ b/tests/scripts/features/varnesting
@@ -9,8 +9,7 @@ variable2 := Hello
 y = $(subst 1,2,$(x))
 z = y
 a := $($($(z)))
-all: 
-	@echo $(a)
+all: ; @echo $(a)
 ',
               '', "Hello\n");
 
@@ -21,15 +20,8 @@ all:
 
 run_make_test('
 VARIABLE = $(eval VARIABLE := echo hi)$(VARIABLE)
-wololo:
-	@$(VARIABLE)
+wololo: ; @$(VARIABLE)
 ',
               '', "hi\n");
 
 1;
-
-
-
-
-
-
diff --git a/tests/scripts/functions/let b/tests/scripts/functions/let
index f1012e56..48aec78f 100644
--- a/tests/scripts/functions/let
+++ b/tests/scripts/functions/let
@@ -39,11 +39,12 @@ all:;@echo 'a=,$a,' 'b=,$b,' 'x=,$x,' 'y=,$y,' 'z=,$z,'
 # We still expand the list and body.
 run_make_test('
 null =
+v = $(let    ,$(info blankvar),abc)
 x = $(let $(null),$(info side-effect),abc)
 y = $(let y,,$ydef)
 
-all: ; @echo $x$y',
-              '', "side-effect\nabcdef\n");
+all: ; @echo $v/$x/$y',
+              '', "blankvar\nside-effect\nabc/abc/def\n");
 
 # The example macro from the manual.
 run_make_test('
diff --git a/tests/scripts/targets/ONESHELL b/tests/scripts/targets/ONESHELL
index 481de088..f9da14b3 100644
--- a/tests/scripts/targets/ONESHELL
+++ b/tests/scripts/targets/ONESHELL
@@ -10,7 +10,7 @@ if ($port_type ne 'W32') {
     # Some shells (*shakes fist at Solaris*) cannot handle multiple flags in
     # separate arguments.
     my $t = `$sh_name -e -c true 2>/dev/null`;
-    my $multi_ok = $? == 0;
+    $multi_ok = $? == 0;
 }
 
 # Simple
diff --git a/tests/test_driver.pl b/tests/test_driver.pl
index 58a782f6..b64fffb6 100644
--- a/tests/test_driver.pl
+++ b/tests/test_driver.pl
@@ -197,7 +197,7 @@ sub toplevel
            'TZ', 'TMPDIR', 'HOME', 'USER', 'LOGNAME', 'PATH',
            'LD_LIBRARY_PATH',
            # *SAN things
-           'ASAN_OPTIONS', 'UBSAN_OPTIONS',
+           'ASAN_OPTIONS', 'UBSAN_OPTIONS', 'LSAN_OPTIONS',
            # Purify things
            'PURIFYOPTIONS',
            # Windows-specific things