From aad2848e8019edc66b662c6de9c9b241fe6d77bf Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Mon, 21 Nov 2016 18:10:15 -0800 Subject: [PATCH] use consistent mechanism to validate var names Builtin commands that validate var names should use a consistent mechanism. I noticed that builtin_read() had it's own custom code that differed slightly from wcsvarname(). Fixes #3569 --- src/builtin.cpp | 38 ++++++++++++++++++++++++------------ src/builtin.h | 1 + src/builtin_set.cpp | 15 ++++---------- src/env_universal_common.cpp | 2 +- src/parse_util.cpp | 2 +- src/wutil.cpp | 1 + 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 1a3d587b9..a3cca58f6 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -78,6 +79,27 @@ bool builtin_data_t::operator<(const builtin_data_t *other) const { return wcscmp(this->name, other->name) < 0; } +static void builtin_append_format(wcstring &str, const wchar_t *fmt, ...) { + va_list ap; + va_start(ap, fmt); + append_formatv(str, fmt, ap); + va_end(ap); +} + +bool builtin_is_valid_varname(const wchar_t *varname, wcstring &errstr, const wchar_t *cmd) { + const wchar_t *invalid_char = wcsvarname(varname); + if (!invalid_char) { + return true; + } + + if (*invalid_char == L'\0') { + builtin_append_format(errstr, BUILTIN_ERR_VARNAME_ZERO, cmd); + } else { + builtin_append_format(errstr, BUILTIN_ERR_VARCHAR, cmd, *invalid_char); + } + return false; +} + /// Counts the number of arguments in the specified null-terminated array int builtin_count_args(const wchar_t *const *argv) { int argc; @@ -1938,21 +1960,13 @@ static int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) } // Verify all variable names. + wcstring errstr; for (i = w.woptind; i < argc; i++) { - wchar_t *src; - - if (!wcslen(argv[i])) { - streams.err.append_format(BUILTIN_ERR_VARNAME_ZERO, argv[0]); + if (!builtin_is_valid_varname(argv[i], errstr, argv[0])) { + streams.err.append(errstr); + builtin_print_help(parser, streams, argv[0], streams.err); return STATUS_BUILTIN_ERROR; } - - for (src = argv[i]; *src; src++) { - if ((!iswalnum(*src)) && (*src != L'_')) { - streams.err.append_format(BUILTIN_ERR_VARCHAR, argv[0], *src); - builtin_print_help(parser, streams, argv[0], streams.err); - return STATUS_BUILTIN_ERROR; - } - } } // The call to reader_readline may change woptind, so we save it away here. diff --git a/src/builtin.h b/src/builtin.h index b76f75a35..f27086799 100644 --- a/src/builtin.h +++ b/src/builtin.h @@ -109,6 +109,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis void builtin_print_help(parser_t &parser, io_streams_t &streams, const wchar_t *cmd, output_stream_t &b); int builtin_count_args(const wchar_t *const *argv); +bool builtin_is_valid_varname(const wchar_t *varname, wcstring &errstr, const wchar_t *cmd); void builtin_unknown_option(parser_t &parser, io_streams_t &streams, const wchar_t *cmd, const wchar_t *opt); diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index 71592d971..a9dd120d4 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -524,18 +524,11 @@ int builtin_set(parser_t &parser, io_streams_t &streams, wchar_t **argv) { *wcschr(dest, L'[') = 0; } - if (!wcslen(dest)) { - free(dest); - streams.err.append_format(BUILTIN_ERR_VARNAME_ZERO, argv[0]); + wcstring errstr; + if (!builtin_is_valid_varname(dest, errstr, argv[0])) { + streams.err.append(errstr); builtin_print_help(parser, streams, argv[0], streams.err); - return 1; - } - - if ((bad_char = wcsvarname(dest))) { - streams.err.append_format(BUILTIN_ERR_VARCHAR, argv[0], *bad_char); - builtin_print_help(parser, streams, argv[0], streams.err); - free(dest); - return 1; + return STATUS_BUILTIN_ERROR; } // Set assignment can work in two modes, either using slices or using the whole array. We detect diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index c19222e63..b04968eff 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -230,7 +230,7 @@ static bool append_file_entry(fish_message_type_t type, const wcstring &key_in, result->push_back(' '); // Append variable name like "fish_color_cwd". - if (wcsvarname(key_in.c_str())) { + if (wcsvarname(key_in)) { debug(0, L"Illegal variable name: '%ls'", key_in.c_str()); success = false; } diff --git a/src/parse_util.cpp b/src/parse_util.cpp index a7825c9e5..35ccbcaa4 100644 --- a/src/parse_util.cpp +++ b/src/parse_util.cpp @@ -847,7 +847,7 @@ void parse_util_expand_variable_error(const wcstring &token, size_t global_token if (closing_bracket != wcstring::npos) { size_t var_start = dollar_pos + 2, var_end = closing_bracket; var_name = wcstring(token, var_start, var_end - var_start); - looks_like_variable = !var_name.empty() && wcsvarname(var_name.c_str()) == NULL; + looks_like_variable = wcsvarname(var_name) == NULL; } if (looks_like_variable) { append_syntax_error( diff --git a/src/wutil.cpp b/src/wutil.cpp index 65220ce3c..2c12068cb 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -484,6 +484,7 @@ int fish_iswgraph(wint_t wc) { /// /// \return null if this is a valid name, and a pointer to the first invalid character otherwise. const wchar_t *wcsvarname(const wchar_t *str) { + if (str[0] == L'\0') return str; while (*str) { if ((!fish_iswalnum(*str)) && (*str != L'_')) { return str;