From c58bd9f569dee6cea1fe0d0d7408076bbd97cb4a Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Mon, 25 Oct 2021 14:41:15 -0300 Subject: [PATCH 1/3] env: don't panic when name is empty --- src/uu/env/src/env.rs | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index aec97fc24..2bf5ed7d1 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -7,7 +7,7 @@ /* last synced with: env (GNU coreutils) 8.13 */ -// spell-checker:ignore (ToDO) chdir execvp progname subcommand subcommands unsets +// spell-checker:ignore (ToDO) chdir execvp progname subcommand subcommands unsets setenv putenv #[macro_use] extern crate clap; @@ -256,7 +256,32 @@ fn run_env(args: impl uucore::Args) -> UResult<()> { // set specified env vars for &(name, val) in &opts.sets { - // FIXME: set_var() panics if name is an empty string + /* + * set_var panics if name is an empty string + * set_var internally calls setenv (on unix at least), while GNU env calls putenv instead. + * + * putenv returns successfully if provided with something like "=a" and modifies the environ + * variable to contain "=a" inside it, effectively modifying the process' current environment + * to contain a malformed string in it. Using GNU's implementation, the command `env =a` + * prints out the malformed string and even invokes the child process with that environment. + * This can be seen by using `env -i =a env` or `env -i =a cat /proc/self/environ` + * + * POSIX.1-2017 doesn't seem to mention what to do if the string is malformed (at least + * not in "Chapter 8, Environment Variables" or in the definition for environ and various + * exec*'s or in the description of env in the "Shell & Utilities" volume). + * + * It also doesn't specify any checks for putenv before modifying the environ variable, which + * is likely why glibc doesn't do so. However, setenv's first argument cannot point to + * an empty string or a string containing '='. + * + * There is no benefit in replicating GNU's env behavior, since it will only modify the + * environment in weird ways + */ + + if name.is_empty() { + show_warning!("no name specified for value {}", val.quote()); + continue; + } env::set_var(name, val); } From f9512e5a90ca52dbc12bd967fc4a6341bcb95e08 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Mon, 25 Oct 2021 14:45:29 -0300 Subject: [PATCH 2/3] tests/env: add empty name test --- tests/by-util/test_env.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/by-util/test_env.rs b/tests/by-util/test_env.rs index 1d76c433d..ecb215f8c 100644 --- a/tests/by-util/test_env.rs +++ b/tests/by-util/test_env.rs @@ -108,6 +108,15 @@ fn test_ignore_environment() { scene.ucmd().arg("-").run().no_stdout(); } +#[test] +fn test_empty_name() { + new_ucmd!() + .arg("-i") + .arg("=xyz") + .run() + .stderr_only("env: warning: no name specified for value 'xyz'"); +} + #[test] fn test_null_delimiter() { let out = new_ucmd!() From 1afc7242a58d0f97a7335c369c4c8536ff80c9fa Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 17:23:34 -0300 Subject: [PATCH 3/3] env: change comment --- src/uu/env/src/env.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 2bf5ed7d1..8967a798a 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -7,7 +7,7 @@ /* last synced with: env (GNU coreutils) 8.13 */ -// spell-checker:ignore (ToDO) chdir execvp progname subcommand subcommands unsets setenv putenv +// spell-checker:ignore (ToDO) chdir execvp progname subcommand subcommands unsets setenv putenv posix_spawnp #[macro_use] extern crate clap; @@ -289,7 +289,12 @@ fn run_env(args: impl uucore::Args) -> UResult<()> { // we need to execute a command let (prog, args) = build_command(&mut opts.program); - // FIXME: this should just use execvp() (no fork()) on Unix-like systems + /* + * On Unix-like systems Command::status either ends up calling either fork or posix_spawnp + * (which ends up calling clone). Keep using the current process would be ideal, but the + * standard library contains many checks and fail-safes to ensure the process ends up being + * created. This is much simpler than dealing with the hassles of calling execvp directly. + */ match Command::new(&*prog).args(args).status() { Ok(exit) if !exit.success() => return Err(exit.code().unwrap().into()), Err(ref err) if err.kind() == io::ErrorKind::NotFound => return Err(127.into()),