From ed78fd2a5f95ed72c0745e15f53fdea6fdde1f91 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 23 Apr 2022 15:17:14 -0700 Subject: [PATCH] Rationalize path-getting This cleans up the path_get_path function which is used to resolve a command name against $PATH, by removing the dependence on errno and being explicit about which error is returned. Should be no user-visible change here. --- src/builtins/command.cpp | 5 +- src/complete.cpp | 10 ++-- src/highlight.cpp | 4 +- src/parse_execution.cpp | 28 +++++----- src/path.cpp | 115 ++++++++++++++++++--------------------- src/path.h | 26 +++++---- 6 files changed, 89 insertions(+), 99 deletions(-) diff --git a/src/builtins/command.cpp b/src/builtins/command.cpp index 58e119023..25cf3d203 100644 --- a/src/builtins/command.cpp +++ b/src/builtins/command.cpp @@ -102,9 +102,8 @@ maybe_t builtin_command(parser_t &parser, io_streams_t &streams, const wcha ++found; } } else { // Either find_path explicitly or just quiet. - wcstring path; - if (path_get_path(command_name, &path, parser.vars())) { - if (!opts.quiet) streams.out.append_format(L"%ls\n", path.c_str()); + if (auto path = path_get_path(command_name, parser.vars())) { + if (!opts.quiet) streams.out.append_format(L"%ls\n", path->c_str()); ++found; } } diff --git a/src/complete.cpp b/src/complete.cpp index 2f97ec5c0..584296d55 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -513,8 +513,9 @@ static completion_entry_t &complete_get_exact_entry(completion_entry_set_t &comp /// Find the full path and commandname from a command string 'str'. static void parse_cmd_string(const wcstring &str, wcstring *path, wcstring *cmd, const environment_t &vars) { - bool found = path_get_path(str, path, vars); - // If the command was not found, 'path' is the empty string. + auto path_result = path_try_get_path(str, vars); + bool found = (path_result.err == 0); + *path = std::move(path_result.path); // Resolve commands that use relative paths because we compare full paths with "complete -p". if (found && !str.empty() && str.at(0) != L'/') { if (auto full_path = wrealpath(*path)) { @@ -888,11 +889,8 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs // Only reload environment variables if builtin_exists returned false, as an optimization if (!head_exists) { head_exists = function_exists_no_autoload(cmd); - // While it may seem like first testing `path_get_path` before resorting to an env lookup - // may be faster, path_get_path can potentially do a lot of FS/IO access, so env.get() + - // function_exists() should still be faster. // Use cmd_orig here as it is potentially pathed. - head_exists = head_exists || path_get_path(cmd_orig, nullptr, ctx.vars); + head_exists = head_exists || path_get_path(cmd_orig, ctx.vars).has_value(); } if (!head_exists) { diff --git a/src/highlight.cpp b/src/highlight.cpp index 4d30fc9b7..ef9a9c900 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -450,7 +450,7 @@ bool autosuggest_validate_from_history(const history_item_t &item, // Not handled specially. Is the command valid? bool cmd_ok = builtin_exists(parsed_command) || function_exists_no_autoload(parsed_command) || - path_get_path(parsed_command, nullptr, ctx.vars); + path_get_path(parsed_command, ctx.vars).has_value(); if (!cmd_ok) { return false; } @@ -1318,7 +1318,7 @@ static bool command_is_valid(const wcstring &cmd, enum statement_decoration_t de if (!is_valid && abbreviation_ok) is_valid = expand_abbreviation(cmd, vars).has_value(); // Regular commands - if (!is_valid && command_ok) is_valid = path_get_path(cmd, nullptr, vars); + if (!is_valid && command_ok) is_valid = path_get_path(cmd, vars).has_value(); // Implicit cd if (!is_valid && implicit_cd_ok) { diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index ea1f444bb..f0eee5ff3 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -735,8 +735,9 @@ end_execution_reason_t parse_execution_context_t::handle_command_not_found( // but this mainly applies to EACCES. We could also feasibly get: // ELOOP // ENAMETOOLONG - return this->report_error(STATUS_NOT_EXECUTABLE, statement, - _(L"Unknown command. '%ls' exists but is not an executable file."), cmd); + return this->report_error( + STATUS_NOT_EXECUTABLE, statement, + _(L"Unknown command. '%ls' exists but is not an executable file."), cmd); } // Handle unrecognized commands with standard command not found handler that can make better @@ -852,14 +853,11 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( // Determine the process type. enum process_type_t process_type = process_type_for_command(statement, cmd); - wcstring path_to_external_command; + get_path_result_t external_cmd{}; if (process_type == process_type_t::external || process_type == process_type_t::exec) { // Determine the actual command. This may be an implicit cd. - bool has_command = path_get_path(cmd, &path_to_external_command, parser->vars()); - - // If there was no command, then we care about the value of errno after checking for it, to - // distinguish between e.g. no file vs permissions problem. - const int no_cmd_err_code = errno; + external_cmd = path_try_get_path(cmd, parser->vars()); + bool has_command = external_cmd.err == 0; // If the specified command does not exist, and is undecorated, try using an implicit cd. if (!has_command && statement.decoration() == statement_decoration_t::none) { @@ -875,7 +873,8 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( if (!has_command && !use_implicit_cd) { // No command. If we're --no-execute return okay - it might be a function. if (no_exec()) return end_execution_reason_t::ok; - return this->handle_command_not_found(path_to_external_command.empty() ? cmd : path_to_external_command, statement, no_cmd_err_code); + return this->handle_command_not_found( + external_cmd.path.empty() ? cmd : external_cmd.path, statement, external_cmd.err); } } @@ -885,7 +884,7 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( if (use_implicit_cd) { // Implicit cd is simple. cmd_args = {L"cd", cmd}; - path_to_external_command.clear(); + external_cmd = get_path_result_t{}; // If we have defined a wrapper around cd, use it, otherwise use the cd builtin. process_type = @@ -917,7 +916,7 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process( proc->type = process_type; proc->set_argv(std::move(cmd_args)); proc->set_redirection_specs(std::move(redirections)); - proc->actual_cmd = std::move(path_to_external_command); + proc->actual_cmd = std::move(external_cmd.path); return end_execution_reason_t::ok; } @@ -1036,8 +1035,7 @@ end_execution_reason_t parse_execution_context_t::populate_not_process( template end_execution_reason_t parse_execution_context_t::populate_block_process( - process_t *proc, const ast::statement_t &statement, - const Type &specific_statement) { + process_t *proc, const ast::statement_t &statement, const Type &specific_statement) { using namespace ast; // We handle block statements by creating process_type_t::block_node, that will bounce back to // us when it's time to execute them. @@ -1154,8 +1152,8 @@ end_execution_reason_t parse_execution_context_t::populate_job_process( *specific_statement.as()); break; case type_t::decorated_statement: { - result = this->populate_plain_process(proc, - *specific_statement.as()); + result = + this->populate_plain_process(proc, *specific_statement.as()); break; } default: { diff --git a/src/path.cpp b/src/path.cpp index 828689a6e..d5a649e0e 100644 --- a/src/path.cpp +++ b/src/path.cpp @@ -29,82 +29,73 @@ #include "wcstringutil.h" #include "wutil.h" // IWYU pragma: keep -// Note that PREFIX is defined in the `Makefile` and is thus defined when this module is compiled. -// This ensures we always default to "/bin", "/usr/bin" and the bin dir defined for the fish -// programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or "/usr/". If the PREFIX -// duplicates /bin or /usr/bin that is harmless other than a trivial amount of time testing a path -// we've already tested. -const wcstring_list_t dflt_pathsv({L"/bin", L"/usr/bin", PREFIX L"/bin"}); +// PREFIX is defined at build time. +const wcstring_list_t kDefaultPath({L"/bin", L"/usr/bin", PREFIX L"/bin"}); + +static get_path_result_t path_get_path_core(const wcstring &cmd, const wcstring_list_t &pathsv, + const environment_t &vars) { + const get_path_result_t noent_res{ENOENT, wcstring{}}; + get_path_result_t result{}; + + /// Test if the given path can be executed. + /// \return 0 on success, an errno value on failure. + auto test_path = [](const wcstring &path) -> int { + std::string narrow = wcs2string(path); + struct stat buff; + if (access(narrow.c_str(), X_OK) != 0 || stat(narrow.c_str(), &buff) != 0) { + return errno; + } + return S_ISREG(buff.st_mode) ? 0 : EACCES; + }; + + // Commands cannot contain NUL byte. + if (cmd.find(L'\0') != wcstring::npos) { + return noent_res; + } -static bool path_get_path_core(const wcstring &cmd, wcstring *out_path, - const maybe_t &bin_path_var) { - // Unix paths can't include a NULL-byte, that's the separator. - // If we let this through, we'd end up checking up to the NULL, - // so we'd get the wrong path. - if (cmd.find(L'\0') != wcstring::npos) return false; // If the command has a slash, it must be an absolute or relative path and thus we don't bother // looking for a matching command. if (cmd.find(L'/') != wcstring::npos) { - std::string narrow = wcs2string(cmd); - if (access(narrow.c_str(), X_OK) != 0) { - return false; - } - - struct stat buff; - if (stat(narrow.c_str(), &buff)) { - return false; - } - if (S_ISREG(buff.st_mode)) { - if (out_path) out_path->assign(cmd); - return true; - } - errno = EACCES; - return false; + wcstring abs_cmd = path_apply_working_directory(cmd, vars.get_pwd_slash()); + int merr = test_path(abs_cmd); + return get_path_result_t{merr, std::move(abs_cmd)}; } - const wcstring_list_t *pathsv; - if (bin_path_var) { - pathsv = &bin_path_var->as_list(); - } else { - pathsv = &dflt_pathsv; - } - - int err = ENOENT; - for (auto next_path : *pathsv) { + get_path_result_t best = noent_res; + wcstring proposed_path; + for (const auto &next_path : pathsv) { if (next_path.empty()) continue; - append_path_component(next_path, cmd); - std::string narrow = wcs2string(next_path); - if (access(narrow.c_str(), X_OK) == 0) { - struct stat buff; - if (stat(narrow.c_str(), &buff) == -1) { - if (errno != EACCES) { - wperror(L"stat"); - } - continue; - } - if (S_ISREG(buff.st_mode)) { - if (out_path) *out_path = std::move(next_path); - return true; - } - err = EACCES; - if (out_path) *out_path = std::move(next_path); - } else if (errno != ENOENT && err == ENOENT) { + proposed_path = next_path; + append_path_component(proposed_path, cmd); + int merr = test_path(proposed_path); + if (merr == 0) { + // We found one. + best = get_path_result_t{merr, std::move(proposed_path)}; + break; + } else if (merr != ENOENT && best.err == ENOENT) { // Keep the first *interesting* error and path around. // ENOENT isn't interesting because not having a file is the normal case. - auto tmperr = errno; // Ignore if the parent directory is already inaccessible. - if (access(wcs2string(wdirname(next_path)).c_str(), X_OK) != 0) continue; - err = tmperr; - if (out_path) *out_path = std::move(next_path); + if (waccess(wdirname(proposed_path), X_OK) == 0) { + best = get_path_result_t{merr, std::move(proposed_path)}; + } } } - - errno = err; - return false; + return best; } -bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars) { - return path_get_path_core(cmd, out_path, vars.get(L"PATH")); +maybe_t path_get_path(const wcstring &cmd, const environment_t &vars) { + auto result = path_try_get_path(cmd, vars); + if (result.err != 0) { + return none(); + } + wcstring path = std::move(result.path); + return path; +} + +get_path_result_t path_try_get_path(const wcstring &cmd, const environment_t &vars) { + auto pathvar = vars.get(L"PATH"); + return path_get_path_core(cmd, pathvar ? pathvar->as_list() : kDefaultPath, vars); } static bool path_is_executable(const std::string &path) { diff --git a/src/path.h b/src/path.h index c20190665..5e0df6e1f 100644 --- a/src/path.h +++ b/src/path.h @@ -44,17 +44,21 @@ dir_remoteness_t path_get_config_remoteness(); class env_stack_t; void path_emit_config_directory_messages(env_stack_t &vars); -/// Finds the path of an executable. -/// -/// Args: -/// cmd - The name of the executable. -/// output_or_NULL - If non-NULL, store the path. -/// vars - The environment variables to use -/// -/// Returns: -/// false if the command can not be found else true. The result -/// should be freed with free(). -bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars); +/// Finds the path of an executable named \p cmd, by looking in $PATH taken from \p vars. +/// \returns the path if found, none if not. +maybe_t path_get_path(const wcstring &cmd, const environment_t &vars); + +/// Finds the path of an executable named \p cmd, by looking in $PATH taken from \p vars. +/// On success, err will be 0 and the path is returned. +/// On failure, we return the "best path" with err set appropriately. +/// For example, if we find a non-executable file, we will return its path and EACCESS. +/// If no candidate path is found, path will be empty and err will be set to ENOENT. +/// Possible err values are taken from access(). +struct get_path_result_t { + int err; + wcstring path; +}; +get_path_result_t path_try_get_path(const wcstring &cmd, const environment_t &vars); /// Return all the paths that match the given command. wcstring_list_t path_get_paths(const wcstring &cmd, const environment_t &vars);