Hi Justin, Thank you very much for your work investigating this issue and the proposed patches.
Justin Swartz <[email protected]> writes: > +#ifdef ENABLE_ACCEPT_ENV > + {"accept-env", 'W', "PATTERN", 0, > + "specify an additional environment variable pattern to accept", GRID}, > +#endif I dislike adding short options that are not supported by other implementations. In coreutils, we have a few short options that conflict with other implementations. I.e., they have the same option but with different behavior. Those are always a bit annoying since they are another portability headache people have to deal with when writing scripts. > +#ifdef ENABLE_ACCEPT_ENV > + case 'W': > + add_allowed_env_pattern (arg); > + break; > +#endif > + Usually the '--enable-*' options are moreso for enabling additional features. The GNU Coding standards says [1]: No ‘--enable’ option should ever cause one feature to replace another. So, I don't think this should be gated behind a './configure --option'. > @@ -90,15 +96,30 @@ is_env_var_allowed (const char *var, const char *val) > { > const char **p; > int allowed = 0; > +#ifdef ENABLE_ACCEPT_ENV > + size_t i; > > - for (p = allowed_env_vars; *p; p++) > + for (i = 0; i < user_env_var_count; i++) > { > - if (fnmatch (*p, var, FNM_NOESCAPE) == 0) > + if (fnmatch (user_env_vars[i], var, FNM_NOESCAPE) == 0) > { > allowed = 1; > break; > } > } This would need the addition of the 'fnmatch' Gnulib module to work around some platform bugs [2]. However, I would prefer avoiding wildcards, etc. and force invocations to be explicit about what they would like to accept. > +#ifdef ENABLE_ACCEPT_ENV > +void > +add_allowed_env_pattern (const char *pattern) > +{ > + if (!pattern || *pattern == 0) > + return; > + > + if (user_env_var_count >= MAX_USER_ENV_VARS) > + { > + syslog (LOG_NOTICE, "Ignoring --accept-env option: limit reached"); > + return; > + } > + > + user_env_vars[user_env_var_count++] = pattern; > +} > +#endif > + Most GNU programs try very hard to avoid arbitrary limits [3]. Some users may want many more than 16 environment variables accepted. I wrote a seperate patch that avoids that issue [4], and also avoids clearenv which is missing on many platforms [5]. We can just do: static char *dummy_environ[] = { NULL }; environ = dummy_environ; It is technically undefined behavior. But GNU coreutils has used it without problems for decades in 'env' without issue. :) Collin [1] https://www.gnu.org/prep/standards/standards.html#How-Configuration-Should-Work [2] https://www.gnu.org/software/gnulib/manual/html_node/fnmatch.html [3] https://www.gnu.org/prep/standards/standards.html#Writing-Robust-Programs [4] https://codeberg.org/inetutils/inetutils/pulls/15 [5] https://www.gnu.org/software/gnulib/manual/html_node/clearenv.html
