From 82b5ba1af4374a537ef2810f3a1154d1b6f46714 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Wed, 9 Aug 2017 11:11:58 -0700 Subject: [PATCH] fix bug in `env_get()` involving empty vars My previous change to eliminate `class var_entry_t` caused me to notice that `env_get()` turned a set but empty var into a missing var. Which is wrong. Fixing that brought to light several other pieces of code that were wrong as a consequence of the aforementioned bug. Another step to fixing issue #4200. --- Makefile.in | 4 ++-- src/builtin_argparse.cpp | 7 +++---- src/builtin_read.cpp | 2 +- src/builtin_set.cpp | 8 +++----- src/common.cpp | 1 - src/env.cpp | 6 ------ src/env.h | 4 ++-- src/expand.cpp | 20 +++++++++----------- src/expand.h | 2 +- src/fish.cpp | 2 +- 10 files changed, 22 insertions(+), 34 deletions(-) diff --git a/Makefile.in b/Makefile.in index 0ecb86acc28f..513eaf33f23d 100644 --- a/Makefile.in +++ b/Makefile.in @@ -353,7 +353,7 @@ test: test-prep install-force test_low_level test_high_level # We want the various tests to run serially so their output doesn't mix # We can do that by adding ordering dependencies based on what goals are being used. # -test_goals := test_low_level test_invocation test_fishscript test_interactive +test_goals := test_low_level test_fishscript test_interactive test_invocation # # The following variables define targets that depend on the tests. If any more targets @@ -374,7 +374,7 @@ test_low_level: fish_tests $(call filter_up_to,test_low_level,$(active_test_goal test_high_level: DESTDIR = $(PWD)/test/root/ test_high_level: prefix = . -test_high_level: test-prep install-force test_invocation test_fishscript test_interactive +test_high_level: test-prep install-force test_fishscript test_interactive test_invocation .PHONY: test_high_level test_invocation: $(call filter_up_to,test_invocation,$(active_test_goals)) diff --git a/src/builtin_argparse.cpp b/src/builtin_argparse.cpp index 45475219d035..55e7afc4d8d0 100644 --- a/src/builtin_argparse.cpp +++ b/src/builtin_argparse.cpp @@ -645,8 +645,7 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) { auto val = list_to_array_val(opt_spec->vals); if (opt_spec->short_flag_valid) { - env_set(var_name_prefix + opt_spec->short_flag, *val == ENV_NULL ? NULL : val->c_str(), - ENV_LOCAL); + env_set(var_name_prefix + opt_spec->short_flag, val->c_str(), ENV_LOCAL); } if (!opt_spec->long_flag.empty()) { // We do a simple replacement of all non alphanum chars rather than calling @@ -655,12 +654,12 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) { for (size_t pos = 0; pos < long_flag.size(); pos++) { if (!iswalnum(long_flag[pos])) long_flag[pos] = L'_'; } - env_set(var_name_prefix + long_flag, *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL); + env_set(var_name_prefix + long_flag, val->c_str(), ENV_LOCAL); } } auto val = list_to_array_val(opts.argv); - env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL); + env_set(L"argv", val->c_str(), ENV_LOCAL); } /// The argparse builtin. This is explicitly not compatible with the BSD or GNU version of this diff --git a/src/builtin_read.cpp b/src/builtin_read.cpp index cfd32ace203b..7d6a0522abbf 100644 --- a/src/builtin_read.cpp +++ b/src/builtin_read.cpp @@ -479,7 +479,7 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) { split_about(buff.begin(), buff.end(), opts.delimiter.begin(), opts.delimiter.end(), &splits, LONG_MAX); auto val = list_to_array_val(splits); - env_set(argv[0], *val == ENV_NULL ? NULL : val->c_str(), opts.place); + env_set(argv[0], val->c_str(), opts.place); } } else { // not array if (!opts.have_delimiter) { diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index 81045103315d..1d1af95a60d7 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -457,17 +457,15 @@ static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int argc, if (!names_only) { env_var_t var = env_get(key, compute_scope(opts)); - if (!var.missing()) { + if (!var.missing_or_empty()) { bool shorten = false; - wcstring val = var.as_string(); + wcstring val = expand_escape_variable(var); if (opts.shorten_ok && val.length() > 64) { shorten = true; val.resize(60); } - - wcstring e_value = expand_escape_variable(val); streams.out.append(L" "); - streams.out.append(e_value); + streams.out.append(val); if (shorten) streams.out.push_back(ellipsis_char); } diff --git a/src/common.cpp b/src/common.cpp index e568043f6047..68cd2274dbf0 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -957,7 +957,6 @@ static void escape_string_script(const wchar_t *orig_in, size_t in_len, wcstring case L'\\': case L'\'': { need_escape = need_complex_escape = 1; - // WTF if (escape_all) out += L'\\'; out += L'\\'; out += *in; break; diff --git a/src/env.cpp b/src/env.cpp index 63389d4ed87e..1fa680d6bcb4 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -1268,12 +1268,6 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) { while (env != NULL) { const env_var_t var = env->find_entry(key); if (!var.missing() && (var.exportv ? search_exported : search_unexported)) { - // The following statement is wrong because ENV_NULL is supposed to mean the var is - // set but has zero elements. Therefore we should not return missing_var. However, - // If this is changed to return `var` without the special-case then unit tests fail. - // Note that tokenize_variable_array() explicitly handles a var whose string - // representation contains only ENV_NULL. - if (var.as_string() == ENV_NULL) return env_var_t::missing_var(); return var; } diff --git a/src/env.h b/src/env.h index aee366f6c02c..5c5890991718 100644 --- a/src/env.h +++ b/src/env.h @@ -89,9 +89,9 @@ class env_var_t { env_var_t(const wchar_t *x) : val(x), is_missing(false), exportv(false) {} env_var_t() : val(L""), is_missing(false), exportv(false) {} - bool empty(void) const { return val.empty(); }; + bool empty(void) const { return val.empty() || val == ENV_NULL; }; bool missing(void) const { return is_missing; } - bool missing_or_empty(void) const { return missing() || val.empty(); } + bool missing_or_empty(void) const { return missing() || empty(); } const wchar_t *c_str(void) const; void to_list(wcstring_list_t &out) const; diff --git a/src/expand.cpp b/src/expand.cpp index f265485d45cc..4a34ba4a2d29 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -27,8 +27,10 @@ #include #include +#include #include // IWYU pragma: keep #include +#include #include #include "common.h" @@ -42,7 +44,6 @@ #include "parse_util.h" #include "path.h" #include "proc.h" -#include "util.h" #include "wildcard.h" #include "wutil.h" // IWYU pragma: keep #ifdef KERN_PROCARGS2 @@ -169,15 +170,13 @@ static int is_quotable(const wchar_t *str) { static int is_quotable(const wcstring &str) { return is_quotable(str.c_str()); } wcstring expand_escape_variable(const env_var_t &var) { - wcstring_list_t lst; wcstring buff; + wcstring_list_t lst; var.to_list(lst); - - size_t size = lst.size(); - if (size == 0) { - buff.append(L"''"); - } else if (size == 1) { + if (lst.size() == 0) { + ; // empty list expands to nothing + } else if (lst.size() == 1) { const wcstring &el = lst.at(0); if (el.find(L' ') != wcstring::npos && is_quotable(el)) { @@ -201,6 +200,7 @@ wcstring expand_escape_variable(const env_var_t &var) { } } } + return buff; } @@ -265,9 +265,7 @@ wcstring process_iterator_t::name_for_pid(pid_t pid) { } args = (char *)malloc(maxarg); - if (args == NULL) { // cppcheck-suppress memleak - return result; - } + if (!args) return result; mib[0] = CTL_KERN; mib[1] = KERN_PROCARGS2; @@ -1584,7 +1582,7 @@ bool fish_xdm_login_hack_hack_hack_hack(std::vector *cmds, int argc } std::map abbreviations; -void update_abbr_cache(const wchar_t *op, const wcstring varname) { +void update_abbr_cache(const wchar_t *op, const wcstring &varname) { wcstring abbr; if (!unescape_string(varname.substr(wcslen(L"_fish_abbr_")), &abbr, 0, STRING_STYLE_VAR)) { debug(1, L"Abbreviation var '%ls' is not correctly encoded, ignoring it.", varname.c_str()); diff --git a/src/expand.h b/src/expand.h index 6643b87426d9..20181a59fd28 100644 --- a/src/expand.h +++ b/src/expand.h @@ -134,7 +134,7 @@ wcstring replace_home_directory_with_tilde(const wcstring &str); /// Abbreviation support. Expand src as an abbreviation, returning true if one was found, false if /// not. If result is not-null, returns the abbreviation by reference. -void update_abbr_cache(const wchar_t *op, const wcstring varnam); +void update_abbr_cache(const wchar_t *op, const wcstring &varname); bool expand_abbreviation(const wcstring &src, wcstring *output); // Terrible hacks diff --git a/src/fish.cpp b/src/fish.cpp index 096805a6e513..a3f45dfe3c06 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -421,7 +421,7 @@ int main(int argc, char **argv) { list.push_back(str2wcstring(*ptr)); } auto val = list_to_array_val(list); - env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), 0); + env_set(L"argv", val->c_str(), ENV_DEFAULT); const wcstring rel_filename = str2wcstring(file);