From 2f33c24a07ab5df55ea7992ef0bf11941b068019 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Thu, 24 Nov 2016 18:43:50 -0800 Subject: [PATCH] fix handling of odd strings by `test` builtin The `test` builtin currently has unexpected behavior with respect to expressions such as `'' -eq 0`. That currently evaluates to true with a return status of zero. This change addresses that oddity while also ensuring that other unusual strings (e.g., numbers with leading and trailing whitespace) are handled consistently. Fixes #3346 --- src/builtin_test.cpp | 6 ++---- src/fish_tests.cpp | 28 +++++++++++++++++++++------- src/wutil.cpp | 31 +++++++++++++++++++++++++++++++ src/wutil.h | 1 + 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/builtin_test.cpp b/src/builtin_test.cpp index 4ed3ce333..f4fe816cb 100644 --- a/src/builtin_test.cpp +++ b/src/builtin_test.cpp @@ -639,10 +639,8 @@ bool parenthetical_expression::evaluate(wcstring_list_t &errors) { // example, should we interpret 0x10 as 0, 10, or 16? Here we use only base 10 and use wcstoll, // which allows for leading + and -, and leading whitespace. This matches bash. static bool parse_number(const wcstring &arg, long long *out) { - const wchar_t *str = arg.c_str(); - wchar_t *endptr = NULL; - *out = wcstoll(str, &endptr, 10); - return endptr && *endptr == L'\0'; + *out = fish_wcstoll(arg.c_str()); + return !errno; } static bool binary_primary_evaluate(test_expressions::token_t token, const wcstring &left, diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 57678f3cb..74608c3c9 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1764,15 +1764,18 @@ static bool run_one_test_test(int expected, wcstring_list_t &lst, bool bracket) static bool run_test_test(int expected, const wcstring &str) { using namespace std; - wcstring_list_t lst; + wcstring_list_t argv; + completion_list_t comps; - wistringstream iss(str); - copy(istream_iterator >(iss), - istream_iterator >(), - back_inserter >(lst)); + // We need to tokenize the string in the same manner a normal shell would do. This is because we + // need to test things like quoted strings that have leading and trailing whitespace. + parser_t::expand_argument_list(str, 0, &comps); + for (completion_list_t::const_iterator it = comps.begin(), end = comps.end(); it != end; ++it) { + argv.push_back(it->completion); + } - bool bracket = run_one_test_test(expected, lst, true); - bool nonbracket = run_one_test_test(expected, lst, false); + bool bracket = run_one_test_test(expected, argv, true); + bool nonbracket = run_one_test_test(expected, argv, false); do_test(bracket == nonbracket); return nonbracket; } @@ -1801,6 +1804,17 @@ static void test_test() { do_test(run_test_test(0, L"0 -eq 0")); do_test(run_test_test(0, L"-1 -eq -1")); do_test(run_test_test(0, L"1 -ne -1")); + do_test(run_test_test(1, L"' 2 ' -ne 2")); + do_test(run_test_test(0, L"' 2' -eq 2")); + do_test(run_test_test(0, L"'2 ' -eq 2")); + do_test(run_test_test(0, L"' 2 ' -eq 2")); + do_test(run_test_test(1, L"' 2x' -eq 2")); + do_test(run_test_test(1, L"'' -eq 0")); + do_test(run_test_test(1, L"'' -ne 0")); + do_test(run_test_test(1, L"' ' -eq 0")); + do_test(run_test_test(1, L"' ' -ne 0")); + do_test(run_test_test(1, L"'x' -eq 0")); + do_test(run_test_test(1, L"'x' -ne 0")); do_test(run_test_test(1, L"-1 -ne -1")); do_test(run_test_test(0, L"abc != def")); do_test(run_test_test(1, L"abc = def")); diff --git a/src/wutil.cpp b/src/wutil.cpp index 3cd7fbdfe..bbbbe497b 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -594,6 +594,37 @@ long fish_wcstol(const wchar_t *str, const wchar_t **endptr, int base) { return result; } +/// An enhanced version of wcstoll(). +/// +/// This is needed because BSD and GNU implementations differ in several ways that make it really +/// annoying to use them in a portable fashion. +/// +/// The caller doesn't have to zero errno. Sets errno to -1 if the int ends with something other +/// than a digit. Leading whitespace is ignored (per the base wcstoll implementation). Trailing +/// whitespace is also ignored. +long long fish_wcstoll(const wchar_t *str, const wchar_t **endptr, int base) { + while (iswspace(*str)) ++str; // skip leading whitespace + if (!*str) { // this is because some implementations don't handle this sensibly + errno = EINVAL; + if (endptr) *endptr = str; + return 0; + } + + errno = 0; + wchar_t *_endptr; + long long result = wcstoll(str, &_endptr, base); + while (iswspace(*_endptr)) ++_endptr; // skip trailing whitespace + if (!errno && *_endptr) { + if (_endptr == str) { + errno = EINVAL; + } else { + errno = -1; + } + } + if (endptr) *endptr = _endptr; + return result; +} + file_id_t file_id_t::file_id_from_stat(const struct stat *buf) { assert(buf != NULL); diff --git a/src/wutil.h b/src/wutil.h index 88ec1ed08..a529de9ce 100644 --- a/src/wutil.h +++ b/src/wutil.h @@ -119,6 +119,7 @@ int fish_wcswidth(const wcstring &str); int fish_wcstoi(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10); long fish_wcstol(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10); +long long fish_wcstoll(const wchar_t *str, const wchar_t **endptr = NULL, int base = 10); /// Class for representing a file's inode. We use this to detect and avoid symlink loops, among /// other things. While an inode / dev pair is sufficient to distinguish co-existing files, Linux