From f2a3a1f9201340325dd459c28cb7bb66d4cd7c3e Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Fri, 29 Oct 2021 20:25:41 -0300 Subject: [PATCH 1/9] printenv: use UResult --- src/uu/printenv/src/printenv.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/uu/printenv/src/printenv.rs b/src/uu/printenv/src/printenv.rs index 5d32cfbcc..1bdd3e8c4 100644 --- a/src/uu/printenv/src/printenv.rs +++ b/src/uu/printenv/src/printenv.rs @@ -9,6 +9,7 @@ use clap::{crate_version, App, Arg}; use std::env; +use uucore::error::UResult; static ABOUT: &str = "Display the values of the specified environment VARIABLE(s), or (with no VARIABLE) display name and value pairs for them all."; @@ -20,7 +21,8 @@ fn usage() -> String { format!("{0} [VARIABLE]... [OPTION]...", uucore::execution_phrase()) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = usage(); let matches = uu_app().usage(&usage[..]).get_matches_from(args); @@ -40,7 +42,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { for (env_var, value) in env::vars() { print!("{}={}{}", env_var, value, separator); } - return 0; + return Ok(()); } for env_var in variables { @@ -48,7 +50,8 @@ pub fn uumain(args: impl uucore::Args) -> i32 { print!("{}{}", var, separator); } } - 0 + + Ok(()) } pub fn uu_app() -> App<'static, 'static> { From b157a73a1fc9f699fc607ce28fdeece8b7cb6026 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Fri, 29 Oct 2021 20:27:06 -0300 Subject: [PATCH 2/9] printenv: change exit code when variable not found GNU printenv has this behavior --- src/uu/printenv/src/printenv.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/uu/printenv/src/printenv.rs b/src/uu/printenv/src/printenv.rs index 1bdd3e8c4..c3f996865 100644 --- a/src/uu/printenv/src/printenv.rs +++ b/src/uu/printenv/src/printenv.rs @@ -45,13 +45,20 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { return Ok(()); } + let mut not_found = false; for env_var in variables { if let Ok(var) = env::var(env_var) { print!("{}{}", var, separator); + } else { + not_found = true; } } - Ok(()) + if not_found { + Err(1.into()) + } else { + Ok(()) + } } pub fn uu_app() -> App<'static, 'static> { From db00fab7e4982c9e2bbbc7451975be171dc5d3e4 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 19:17:54 -0300 Subject: [PATCH 3/9] env: use UResult everywhere --- src/uu/env/src/env.rs | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 8967a798a..4fdf825dc 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -23,7 +23,7 @@ use std::io::{self, Write}; use std::iter::Iterator; use std::process::Command; use uucore::display::Quotable; -use uucore::error::{UResult, USimpleError}; +use uucore::error::{UResult, USimpleError, UUsageError}; const USAGE: &str = "env [OPTION]... [-] [NAME=VALUE]... [COMMAND [ARG]...]"; const AFTER_HELP: &str = "\ @@ -50,7 +50,7 @@ fn print_env(null: bool) { } } -fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> Result { +fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> UResult { // is it a NAME=VALUE like opt ? if let Some(idx) = opt.find('=') { // yes, so push name, value pair @@ -64,17 +64,12 @@ fn parse_name_value_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> Result(opts: &mut Options<'a>, opt: &'a str) -> Result<(), i32> { +fn parse_program_opt<'a>(opts: &mut Options<'a>, opt: &'a str) -> UResult<()> { if opts.null { - eprintln!( - "{}: cannot specify --null (-0) with command", - uucore::util_name() - ); - eprintln!( - "Type \"{} --help\" for detailed information", - uucore::execution_phrase() - ); - Err(1) + Err(UUsageError::new( + 125, + "cannot specify --null (-0) with command".to_string(), + )) } else { opts.program.push(opt); Ok(()) @@ -93,10 +88,8 @@ fn load_config_file(opts: &mut Options) -> UResult<()> { Ini::load_from_file(file) }; - let conf = conf.map_err(|error| { - show_error!("{}: {}", file.maybe_quote(), error); - 1 - })?; + let conf = + conf.map_err(|e| USimpleError::new(1, format!("{}: {}", file.maybe_quote(), e)))?; for (_, prop) in &conf { // ignore all INI section lines (treat them as comments) From 3d74e7b452e8473ebc3cc7afefa82e21c5bb030d Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 19:22:03 -0300 Subject: [PATCH 4/9] env: prevent panic when unsetting invalid variable --- src/uu/env/src/env.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 4fdf825dc..042d2c380 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -244,6 +244,13 @@ fn run_env(args: impl uucore::Args) -> UResult<()> { // unset specified env vars for name in &opts.unsets { + if name.is_empty() || name.contains(0 as char) || name.contains('=') { + return Err(USimpleError::new( + 125, + format!("cannot unset {}: Invalid argument", name.quote()), + )); + } + env::remove_var(name); } From c44b5952b8ae2fef24f41a428a8de554964558f8 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 19:32:41 -0300 Subject: [PATCH 5/9] tests/env: add unsetting invalid variables test --- tests/by-util/test_env.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_env.rs b/tests/by-util/test_env.rs index ecb215f8c..0d9d94b01 100644 --- a/tests/by-util/test_env.rs +++ b/tests/by-util/test_env.rs @@ -1,4 +1,4 @@ -// spell-checker:ignore (words) bamf chdir +// spell-checker:ignore (words) bamf chdir rlimit prlimit #[cfg(not(windows))] use std::fs; @@ -80,6 +80,20 @@ fn test_combined_file_set_unset() { ); } +#[test] +fn test_unset_invalid_variables() { + use uucore::display::Quotable; + + // Cannot test input with \0 in it, since output will also contain \0. rlimit::prlimit fails + // with this error: Error { kind: InvalidInput, message: "nul byte found in provided data" } + for var in &["", "a=b"] { + new_ucmd!().arg("-u").arg(var).run().stderr_only(format!( + "env: cannot unset {}: Invalid argument", + var.quote() + )); + } +} + #[test] fn test_single_name_value_pair() { let out = new_ucmd!().arg("FOO=bar").run(); From 013405d1e60761f02aeeca9b007b3c91df0ab60b Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 19:18:59 -0300 Subject: [PATCH 6/9] env: change -c to -C --- src/uu/env/src/env.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 042d2c380..6d5431aa9 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -131,7 +131,7 @@ pub fn uu_app() -> App<'static, 'static> { .long("ignore-environment") .help("start with an empty environment")) .arg(Arg::with_name("chdir") - .short("c") + .short("C") // GNU env compatibility .long("chdir") .takes_value(true) .number_of_values(1) From 8ad95c375ad6376143bd8ba938525bb4c7894ccb Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 19:53:40 -0300 Subject: [PATCH 7/9] env: force specifying command with --chdir --- src/uu/env/src/env.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 6d5431aa9..835c8766c 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -229,6 +229,14 @@ fn run_env(args: impl uucore::Args) -> UResult<()> { } } + // GNU env tests this behavior + if opts.program.is_empty() && running_directory.is_some() { + return Err(UUsageError::new( + 125, + "must specify command with --chdir (-C)".to_string(), + )); + } + // NOTE: we manually set and unset the env vars below rather than using Command::env() to more // easily handle the case where no command is given From a290c77cfc7276c7b2ae657180ae735b3d4ec752 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 20:25:48 -0300 Subject: [PATCH 8/9] env: add contributor --- src/uu/env/src/env.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/uu/env/src/env.rs b/src/uu/env/src/env.rs index 835c8766c..346bf4c8e 100644 --- a/src/uu/env/src/env.rs +++ b/src/uu/env/src/env.rs @@ -1,6 +1,7 @@ // This file is part of the uutils coreutils package. // // (c) Jordi Boggiano +// (c) Thomas Queiroz // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. From 124f92984809a740e2a5da362f6c0c727e71b05b Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 2 Nov 2021 20:06:59 -0300 Subject: [PATCH 9/9] tests/env: change Windows test_change_directory Invoke `cmd.exe /C cd` to determine the current working directory instead of relying on the output of environment variables. --- tests/by-util/test_env.rs | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tests/by-util/test_env.rs b/tests/by-util/test_env.rs index 0d9d94b01..135ee72ef 100644 --- a/tests/by-util/test_env.rs +++ b/tests/by-util/test_env.rs @@ -1,7 +1,4 @@ -// spell-checker:ignore (words) bamf chdir rlimit prlimit - -#[cfg(not(windows))] -use std::fs; +// spell-checker:ignore (words) bamf chdir rlimit prlimit COMSPEC use crate::common::util::*; use std::env; @@ -177,7 +174,7 @@ fn test_fail_null_with_program() { fn test_change_directory() { let scene = TestScenario::new(util_name!()); let temporary_directory = tempdir().unwrap(); - let temporary_path = fs::canonicalize(temporary_directory.path()).unwrap(); + let temporary_path = std::fs::canonicalize(temporary_directory.path()).unwrap(); assert_ne!(env::current_dir().unwrap(), temporary_path); // command to print out current working directory @@ -193,27 +190,36 @@ fn test_change_directory() { assert_eq!(out.trim(), temporary_path.as_os_str()) } -// no way to consistently get "current working directory", `cd` doesn't work @ CI -// instead, we test that the unique temporary directory appears somewhere in the printed variables #[cfg(windows)] #[test] fn test_change_directory() { let scene = TestScenario::new(util_name!()); let temporary_directory = tempdir().unwrap(); - let temporary_path = temporary_directory.path(); - assert_ne!(env::current_dir().unwrap(), temporary_path); + let temporary_path = temporary_directory.path(); + let temporary_path = temporary_path + .strip_prefix(r"\\?\") + .unwrap_or(temporary_path); + + let env_cd = env::current_dir().unwrap(); + let env_cd = env_cd.strip_prefix(r"\\?\").unwrap_or(&env_cd); + + assert_ne!(env_cd, temporary_path); + + // COMSPEC is a variable that contains the full path to cmd.exe + let cmd_path = env::var("COMSPEC").unwrap(); + + // command to print out current working directory + let pwd = [&*cmd_path, "/C", "cd"]; let out = scene .ucmd() .arg("--chdir") .arg(&temporary_path) + .args(&pwd) .succeeds() .stdout_move_str(); - - assert!(!out - .lines() - .any(|line| line.ends_with(temporary_path.file_name().unwrap().to_str().unwrap()))); + assert_eq!(out.trim(), temporary_path.as_os_str()) } #[test]