From 16e14b4114c0c9cc14d15e818bf22588a79e39da Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Sun, 24 Jul 2022 14:14:32 -0400 Subject: [PATCH] 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 | 7 ++ src/function.c | 2 +- src/job.c | 2 +- src/main.c | 6 +- src/makeint.h | 3 + src/os.h | 9 +- src/posixos.c | 13 ++- src/variable.c | 147 ++++++++++++++++++++++-------- src/variable.h | 4 +- src/w32/w32os.c | 7 ++ tests/scripts/variables/MAKEFLAGS | 26 +++++- 11 files changed, 175 insertions(+), 51 deletions(-) diff --git a/NEWS b/NEWS index 8c1e271b..752de7d0 100644 --- a/NEWS +++ b/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. diff --git a/src/function.c b/src/function.c index 1f490fa2..a2fd90a7 100644 --- a/src/function.c +++ b/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]); diff --git a/src/job.c b/src/job.c index 8837b744..c1e53de7 100644 --- a/src/job.c +++ b/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) diff --git a/src/main.c b/src/main.c index 656eaec6..e3658f05 100644 --- a/src/main.c +++ b/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" }, diff --git a/src/makeint.h b/src/makeint.h index e355051d..e711a3c8 100644 --- a/src/makeint.h +++ b/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; diff --git a/src/os.h b/src/os.h index ae73da80..19f61c19 100644 --- a/src/os.h +++ b/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); diff --git a/src/posixos.c b/src/posixos.c index 9eecfcde..de19c889 100644 --- a/src/posixos.c +++ b/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) { diff --git a/src/variable.c b/src/variable.c index c51f41dc..30f4424a 100644 --- a/src/variable.c +++ b/src/variable.c @@ -24,6 +24,7 @@ this program. If not, see . */ #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); diff --git a/src/variable.h b/src/variable.h index 22e2d687..a28749c6 100644 --- a/src/variable.h +++ b/src/variable.h @@ -16,6 +16,8 @@ this program. If not, see . */ #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); diff --git a/src/w32/w32os.c b/src/w32/w32os.c index 54265c8b..56d6972a 100644 --- a/src/w32/w32os.c +++ b/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 () { diff --git a/tests/scripts/variables/MAKEFLAGS b/tests/scripts/variables/MAKEFLAGS index d5fa0d28..a41f1cf0 100644 --- a/tests/scripts/variables/MAKEFLAGS +++ b/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;