Disable the jobserver in non-recursive children

Savannah issues such as SV 57242 and SV 62397 show how passing
references to closed file descriptors via the --jobserver-auth option
in MAKEFLAGS can lead to problematic outcomes.

When computing the child environment for a non-recursive shell, add
an extra option to MAKEFLAGS to disable the file descriptors for the
jobserver.

Unfortunately this doesn't modify the value of the make variable
MAKEFLAGS, it only modifies the value of the sub-shell environment
variable MAKEFLAGS.  This can lead to confusion if the user is not
considering the distinction.

* src/makeint.h: Publish the jobserver-auth value.  Add a global
definition of the name of the command line option.
* src/os.h (jobserver_get_invalid_auth): New function to return a
string invalidating the jobserver-auth option.
* src/w32/w32os.c (jobserver_get_invaid_auth): Implement it.  On
Windows we use a semaphore so there's no need to invalidate.
* src/posixos.c (jobserver_parse_auth): If we parse the invalid
auth string, don't set up the jobserver.
(jobserver_get_invalid_auth): Return an invalid option.
* src/variable.h (target_environment): Specify if the target
environment is for a recursive shell or non-recursive shell.
* src/variable.c (target_environment): Move checking for MAKELEVEL
into the loop rather than doing it at the end.
Along with this, check for MAKEFLAGS and MFLAGS, and update them
based on whether we're invoking a recursive or non-recursive child,
and also on whether it's necessary to invalidate the jobserver.
* src/function.c (func_shell_base): Shell functions can never be
recursive to pass 0 to target_environment().
* src/job.c (start_job_command): Specify whether the child is
recursive when calling target_environment().
* src/main.c: Export jobserver_auth.  sync_mutex doesn't need to
be exported.  Use the global definition for the option name.
* tests/scripts/variables/MAKEFLAGS: Add tests for $MAKEFLAGS.

NEWS

@@ -107,6 +107,13 @@ https://sv.gnu.org/bugs/index.php?group=make&report_id=111&fix_release_id=109&se
 * Special targets like .POSIX are detected upon definition, ensuring that any
   change in behavior takes effect immediately, before the next line is parsed.
 
+* When the jobserver is enabled and GNU make decides it is invoking a non-make
+  sub-process and closes the jobserver pipes, it will now add a new option to
+  the MAKEFLAGS environment variable that disables the jobserver.
+  This prevents sub-processes that invoke make from accidentally using other
+  open file descriptors as jobserver pipes.  For more information see
+  https://savannah.gnu.org/bugs/?57242 and https://savannah.gnu.org/bugs/?62397
+
 * A long-standing issue with the directory cache has been resolved: changes
   made as a side-effect of some other target's recipe are now noticed as
   expected.

src/function.c

@@ -1876,7 +1876,7 @@ func_shell_base (char *o, char **argv, int trim_newlines)
   errfd = (output_context && output_context->err >= 0
            ? output_context->err : FD_STDERR);
 
-  child.environment = target_environment (NULL);
+  child.environment = target_environment (NULL, 0);
 
 #if defined(__MSDOS__)
   fpipe = msdos_openpipe (pipedes, &pid, argv[0]);

src/job.c

@@ -1430,7 +1430,7 @@ start_job_command (struct child *child)
 #ifndef _AMIGA
   /* Set up the environment for the child.  */
   if (child->environment == 0)
-    child->environment = target_environment (child->file);
+    child->environment = target_environment (child->file, child->recursive);
 #endif
 
 #if !defined(__MSDOS__) && !defined(_AMIGA) && !defined(WINDOWS32)

src/main.c

@@ -232,7 +232,7 @@ static const int inf_jobs = 0;
 
 /* Authorization for the jobserver.  */
 
-static char *jobserver_auth = NULL;
+char *jobserver_auth = NULL;
 
 /* Shuffle mode for goals and prerequisites.  */
 
@@ -241,7 +241,7 @@ static char *shuffle_mode = NULL;
 /* Handle for the mutex used on Windows to synchronize output of our
    children under -O.  */
 
-char *sync_mutex = NULL;
+static char *sync_mutex = NULL;
 
 /* Maximum load average at which multiple jobs will be run.
    Negative values mean unlimited, while zero means limit to
@@ -475,7 +475,7 @@ static const struct command_switch switches[] =
 
     /* These are long-style options.  */
     { CHAR_MAX+1, strlist, &db_flags, 1, 1, 0, "basic", 0, "debug" },
-    { CHAR_MAX+2, string, &jobserver_auth, 1, 1, 0, 0, 0, "jobserver-auth" },
+    { CHAR_MAX+2, string, &jobserver_auth, 1, 1, 0, 0, 0, JOBSERVER_AUTH_OPT },
     { CHAR_MAX+3, flag, &trace_flag, 1, 1, 0, 0, 0, "trace" },
     { CHAR_MAX+4, flag_off, &print_directory_flag, 1, 1, 0, 0,
       &default_print_directory_flag, "no-print-directory" },

src/makeint.h

@@ -710,6 +710,9 @@ extern int batch_mode_shell;
 #define RECIPEPREFIX_DEFAULT    '\t'
 extern char cmd_prefix;
 
+#define JOBSERVER_AUTH_OPT      "jobserver-auth"
+
+extern char *jobserver_auth;
 extern unsigned int job_slots;
 extern double max_load_average;
 

src/os.h

@@ -25,11 +25,16 @@ unsigned int jobserver_enabled (void);
 /* Called in the master instance to set up the jobserver initially.  */
 unsigned int jobserver_setup (int job_slots);
 
-/* Called in a child instance to connect to the jobserver.  */
+/* Called in a child instance to connect to the jobserver.
+   Return 1 if we got a valid auth, else 0.  */
 unsigned int jobserver_parse_auth (const char* auth);
 
 /* Returns an allocated buffer used to pass to child instances.  */
-char *jobserver_get_auth (void);
+char *jobserver_get_auth ();
+
+/* Returns a pointer to a static string used to indicate that the child
+   cannot access the jobserver, or NULL if it always can.  */
+const char *jobserver_get_invalid_auth ();
 
 /* Clear this instance's jobserver configuration.  */
 void jobserver_clear (void);

src/posixos.c

@@ -121,6 +121,9 @@ jobserver_parse_auth (const char *auth)
   DB (DB_JOBS,
       (_("Jobserver client (fds %d,%d)\n"), job_fds[0], job_fds[1]));
 
+  if (job_fds[0] == -2 || job_fds[1] == -2)
+    return 0;
+
 #ifdef HAVE_FCNTL_H
 # define FD_OK(_f) (fcntl ((_f), F_GETFD) != -1)
 #else
@@ -154,13 +157,21 @@ jobserver_parse_auth (const char *auth)
 }
 
 char *
-jobserver_get_auth (void)
+jobserver_get_auth ()
 {
   char *auth = xmalloc ((INTSTR_LENGTH * 2) + 2);
   sprintf (auth, "%d,%d", job_fds[0], job_fds[1]);
   return auth;
 }
 
+const char *
+jobserver_get_invalid_auth ()
+{
+  /* It's not really great that we are assuming the command line option
+     here but other alternatives are also gross.  */
+  return " --" JOBSERVER_AUTH_OPT "=-2,-2";
+}
+
 unsigned int
 jobserver_enabled (void)
 {

src/variable.c

@@ -24,6 +24,7 @@ this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "job.h"
 #include "commands.h"
 #include "variable.h"
+#include "os.h"
 #include "rule.h"
 #ifdef WINDOWS32
 #include "pathstuff.h"
@@ -379,7 +380,6 @@ lookup_special_var (struct variable *var)
 {
   static unsigned long last_changenum = 0;
 
-
   /* This one actually turns out to be very hard, due to the way the parser
      records targets.  The way it works is that target information is collected
      internally until make knows the target is completely specified.  Only when
@@ -1018,21 +1018,30 @@ should_export (const struct variable *v)
 
 /* Create a new environment for FILE's commands.
    If FILE is nil, this is for the 'shell' function.
-   The child's MAKELEVEL variable is incremented.  */
+   The child's MAKELEVEL variable is incremented.
+   If recursive is true then we're running a recursive make, else not.  */
 
 char **
-target_environment (struct file *file)
+target_environment (struct file *file, int recursive)
 {
   struct variable_set_list *set_list;
   struct variable_set_list *s;
   struct hash_table table;
   struct variable **v_slot;
   struct variable **v_end;
-  struct variable makelevel_key;
   char **result_0;
   char **result;
+  const char *invalid = NULL;
   /* If we got no value from the environment then never add the default.  */
   int added_SHELL = shell_var.value == 0;
+  int found_makelevel = 0;
+  int found_mflags = 0;
+  int found_makeflags = 0;
+
+  /* We need to update makeflags if (a) we're not recurive, (b) jobserver_auth
+     is enabled, and (c) we need to add invalidation.  */
+  if (!recursive && jobserver_auth)
+    invalid = jobserver_get_invalid_auth ();
 
   if (file == 0)
     set_list = current_variable_set_list;
@@ -1067,17 +1076,11 @@ target_environment (struct file *file)
                   hash_insert_at (&table, v, evslot);
               }
             else if ((*evslot)->export == v_default)
-              {
-                /* We already have a variable but we don't know its status.  */
-                (*evslot)->export = v->export;
-              }
+              /* We already have a variable but we don't know its status.  */
+              (*evslot)->export = v->export;
           }
     }
 
-  makelevel_key.name = (char *)MAKELEVEL_NAME;
-  makelevel_key.length = MAKELEVEL_LENGTH;
-  hash_delete (&table, &makelevel_key);
-
   result = result_0 = xmalloc ((table.ht_fill + 3) * sizeof (char *));
 
   v_slot = (struct variable **) table.ht_vec;
@@ -1086,16 +1089,14 @@ target_environment (struct file *file)
     if (! HASH_VACANT (*v_slot))
       {
         struct variable *v = *v_slot;
+        char *value = v->value;
+        char *cp = NULL;
 
         /* This might be here because it was a target-specific variable that
            we didn't know the status of when we added it.  */
         if (! should_export (v))
           continue;
 
-        /* If this is the SHELL variable remember we already added it.  */
-        if (!added_SHELL && streq (v->name, "SHELL"))
-          added_SHELL = 1;
-
         /* If V is recursively expanded and didn't come from the environment,
            expand its value.  If it came from the environment, it should
            go back into the environment unchanged.  */
@@ -1104,37 +1105,109 @@ target_environment (struct file *file)
             /* If V is being recursively expanded and this is for a shell
                function, just skip it.  */
             if (v->expanding && file == NULL)
-              DB (DB_VERBOSE, (_("%s:%lu: Skipping export of %s to shell function due to recursive expansion"),
-                               v->fileinfo.filenm, v->fileinfo.lineno, v->name));
-            else
               {
-                char *value = recursively_expand_for_file (v, file);
-#ifdef WINDOWS32
-                if (strcmp (v->name, "Path") == 0 ||
-                    strcmp (v->name, "PATH") == 0)
-                  convert_Path_to_windows32 (value, ';');
-#endif
-                *result++ = xstrdup (concat (3, v->name, "=", value));
-                free (value);
+                DB (DB_VERBOSE, (_("%s:%lu: Skipping export of %s to shell function due to recursive expansion"),
+                                 v->fileinfo.filenm, v->fileinfo.lineno, v->name));
+                continue;
+              }
+
+            value = cp = recursively_expand_for_file (v, file);
+          }
+
+        /* If this is the SHELL variable remember we already added it.  */
+        if (!added_SHELL && streq (v->name, "SHELL"))
+          {
+            added_SHELL = 1;
+            goto setit;
+          }
+
+        /* If this is MAKELEVEL, update it.  */
+        if (!found_makelevel && streq (v->name, MAKELEVEL_NAME))
+          {
+            char val[INTSTR_LENGTH + 1];
+            sprintf (val, "%u", makelevel + 1);
+            free (cp);
+            value = cp = xstrdup (val);
+            found_makelevel = 1;
+            goto setit;
+          }
+
+        /* If we need to reset jobserver, check for MAKEFLAGS / MFLAGS.  */
+        if (invalid)
+          {
+            if (!found_makeflags && streq (v->name, MAKEFLAGS_NAME))
+              {
+                char *mf;
+                char *vars;
+                found_makeflags = 1;
+
+                if (!strstr (value, " --" JOBSERVER_AUTH_OPT "="))
+                  goto setit;
+
+                /* The invalid option must come before variable overrides.  */
+                vars = strstr (value, " -- ");
+                if (!vars)
+                  mf = xstrdup (concat (2, value, invalid));
+                else
+                  {
+                    size_t lf = vars - value;
+                    size_t li = strlen (invalid);
+                    mf = xmalloc (strlen (value) + li + 1);
+                    strcpy (mempcpy (mempcpy (mf, value, lf), invalid, li),
+                            vars);
+                  }
+                free (cp);
+                value = cp = mf;
+                if (found_mflags)
+                  invalid = NULL;
+                goto setit;
+              }
+
+            if (!found_mflags && streq (v->name, "MFLAGS"))
+              {
+                const char *mf;
+                found_mflags = 1;
+
+                if (!strstr (value, " --" JOBSERVER_AUTH_OPT "="))
+                  goto setit;
+
+                if (v->origin != o_env)
+                  goto setit;
+                mf = concat (2, value, invalid);
+                free (cp);
+                value = cp = xstrdup (mf);
+                if (found_makeflags)
+                  invalid = NULL;
+                goto setit;
               }
           }
-        else
-          {
+
 #ifdef WINDOWS32
-            if (strcmp (v->name, "Path") == 0 ||
-                strcmp (v->name, "PATH") == 0)
-              convert_Path_to_windows32 (v->value, ';');
-#endif
-            *result++ = xstrdup (concat (3, v->name, "=", v->value));
+        if (streq (v->name, "Path") || streq (v->name, "PATH"))
+          {
+            if (!cp)
+              cp = xstrdup (value);
+            value = convert_Path_to_windows32 (cp, ';');
+            goto setit;
           }
+#endif
+
+      setit:
+        *result++ = xstrdup (concat (3, v->name, "=", value));
+        free (cp);
       }
 
   if (!added_SHELL)
     *result++ = xstrdup (concat (3, shell_var.name, "=", shell_var.value));
 
-  *result = xmalloc (100);
-  sprintf (*result, "%s=%u", MAKELEVEL_NAME, makelevel + 1);
-  *++result = 0;
+  if (!found_makelevel)
+    {
+      char val[MAKELEVEL_LENGTH + 1 + INTSTR_LENGTH + 1];
+      sprintf (val, "%s=%u", MAKELEVEL_NAME, makelevel + 1);
+      *result++ = xstrdup (val);
+    }
+
+  *result = NULL;
 
   hash_free (&table, 0);
 

src/variable.h

@@ -16,6 +16,8 @@ this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "hash.h"
 
+struct file;
+
 /* Codes in a variable definition saying where the definition came from.
    Increasing numeric values signify less-overridable definitions.  */
 enum variable_origin
@@ -235,7 +237,7 @@ void undefine_variable_in_set (const char *name, size_t length,
                                        (int)(l), (n));                  \
                               }while(0)
 
-char **target_environment (struct file *file);
+char **target_environment (struct file *file, int recursive);
 
 struct pattern_var *create_pattern_var (const char *target,
                                         const char *suffix);

src/w32/w32os.c

@@ -90,6 +90,13 @@ jobserver_get_auth ()
   return xstrdup (jobserver_semaphore_name);
 }
 
+const char *
+jobserver_get_invalid_auth ()
+{
+  /* Because we're using a semaphore we don't need to invalidate.  */
+  return NULL;
+}
+
 unsigned int
 jobserver_enabled ()
 {

tests/scripts/variables/MAKEFLAGS

@@ -6,17 +6,17 @@ $details = "DETAILS";
 
 # Normal flags aren't prefixed with "-"
 run_make_test(q!
-all: ; @echo $(MAKEFLAGS)
+all: ; @echo /$(MAKEFLAGS)/
 !,
-              '-e -r -R', 'erR');
+              '-e -r -R', '/erR/');
 
 # Long arguments mean everything is prefixed with "-"
 run_make_test(q!
-all: ; @echo $(MAKEFLAGS)
+all: ; @echo /$(MAKEFLAGS)/
 !,
               '--no-print-directory -e -r -R --trace', "#MAKEFILE#:2: update target 'all' due to: target does not exist
-echo erR --trace --no-print-directory
-erR --trace --no-print-directory");
+echo /erR --trace --no-print-directory/
+/erR --trace --no-print-directory/");
 
 
 # Recursive invocations of make should accumulate MAKEFLAGS values.
@@ -139,4 +139,20 @@ all:; $(info makeflags='$(XX)')
     '-Onone -l2.5 -l2.5 -Onone -I/tmp -iknqrs -i -n -s -k -I/tmp',
     "makeflags='iknqrsw -I/tmp -I/tmp -Onone -Onone -l2.5 -l2.5 --no-print-directory'");
 
+# Verify that command line arguments are included in MAKEFLAGS
+run_make_test(q!
+all: ; @echo $(MAKEFLAGS)
+!,
+              '-e FOO=bar -r -R', 'erR -- FOO=bar');
+
+# Long arguments mean everything is prefixed with "-"
+run_make_test(q!
+all: ; @echo /$(MAKEFLAGS)/
+!,
+              '--no-print-directory -e -r -R --trace FOO=bar',
+              "#MAKEFILE#:2: update target 'all' due to: target does not exist
+echo /erR --trace --no-print-directory -- FOO=bar/
+/erR --trace --no-print-directory -- FOO=bar/");
+
+
 1;