From 48b59cc194b2bc708312307cda4218b41a911988 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 28 Jun 2020 23:06:41 -0500 Subject: [PATCH 1/2] Fix detection of executables with 007 mode It's not surprising that this never came up, as I cannot imagine a more useless chmod value. Perhaps in the context of nologin or something? *shrug* --- src/wildcard.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wildcard.cpp b/src/wildcard.cpp index 4c22a0b5d..591478e75 100644 --- a/src/wildcard.cpp +++ b/src/wildcard.cpp @@ -403,7 +403,7 @@ static const wchar_t *file_get_desc(const wcstring &filename, int lstat_res, return COMPLETE_SOCKET_DESC; } else if (S_ISDIR(buf.st_mode)) { return COMPLETE_DIRECTORY_DESC; - } else if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXGRP) && waccess(filename, X_OK) == 0) { + } else if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH) && waccess(filename, X_OK) == 0) { // Weird group permissions and other such issues make it non-trivial to find out if we can // actually execute a file using the result from stat. It is much safer to use the access // function, since it tells us exactly what we want to know. From 0b55f08de23f818cc4d839dace6926d30cf941dc Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Wed, 24 Jun 2020 15:36:03 -0500 Subject: [PATCH 2/2] Speed up executable command completions This brings down the number of syscalls per potential completion result from three or four to just one. --- src/wildcard.cpp | 75 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 13 deletions(-) diff --git a/src/wildcard.cpp b/src/wildcard.cpp index 591478e75..5dc2bab64 100644 --- a/src/wildcard.cpp +++ b/src/wildcard.cpp @@ -357,6 +357,62 @@ bool wildcard_match(const wcstring &str, const wcstring &wc, bool leading_dots_f return match != fuzzy_match_none; } +static int fast_waccess(const struct stat &stat_buf, uint8_t mode) { + // Cache the effective user id and group id of our own shell process. These can't change on us + // because we don't change them. + static const uid_t euid = geteuid(); + static const gid_t egid = getegid(); + + // Cache a list of our group memberships. + static const std::vector groups = ([&]() { + std::vector groups; + while (true) { + int ngroups = getgroups(0, nullptr); + // It is not defined if getgroups(2) includes the effective group of the calling process + groups.reserve(ngroups + 1); + groups.resize(ngroups, 0); + if (getgroups(groups.size(), groups.data()) == -1) { + if (errno == EINVAL) { + // Race condition, ngroups has changed between the two getgroups() calls + continue; + } + wperror(L"getgroups"); + } + break; + } + + groups.push_back(egid); + std::sort(groups.begin(), groups.end()); + return groups; + })(); + + bool have_suid = (stat_buf.st_mode & S_ISUID); + if (euid == stat_buf.st_uid || have_suid) { + // Check permissions granted to owner + if (((stat_buf.st_mode & S_IRWXU) >> 6) & mode) { + return 0; + } + } + bool have_sgid = (stat_buf.st_mode & S_ISGID); + auto binsearch = std::lower_bound(groups.begin(), groups.end(), stat_buf.st_gid); + bool have_group = binsearch != groups.end() && !(stat_buf.st_gid < *binsearch); + if (have_group || have_sgid) { + // Check permissions granted to group + if (((stat_buf.st_mode & S_IRWXG) >> 3) & mode) { + return 0; + } + } + if (euid != stat_buf.st_uid && !have_group) { + // Check permissions granted to other + if ((stat_buf.st_mode & S_IRWXO) & mode) { + return 0; + } + } + + return -1; +} + + /// Obtain a description string for the file specified by the filename. /// /// The returned value is a string constant and should not be free'd. @@ -367,9 +423,8 @@ bool wildcard_match(const wcstring &str, const wcstring &wc, bool leading_dots_f /// \param stat_res The result of calling stat on the file /// \param buf The struct buf output of calling stat on the file /// \param err The errno value after a failed stat call on the file. -static const wchar_t *file_get_desc(const wcstring &filename, int lstat_res, - const struct stat &lbuf, int stat_res, const struct stat &buf, - int err) { +static const wchar_t *file_get_desc(int lstat_res, const struct stat &lbuf, int stat_res, + const struct stat &buf, int err) { if (lstat_res) { return COMPLETE_FILE_DESC; } @@ -379,10 +434,7 @@ static const wchar_t *file_get_desc(const wcstring &filename, int lstat_res, if (S_ISDIR(buf.st_mode)) { return COMPLETE_DIRECTORY_SYMLINK_DESC; } - if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH) && waccess(filename, X_OK) == 0) { - // Weird group permissions and other such issues make it non-trivial to find out if - // we can actually execute a file using the result from stat. It is much safer to - // use the access function, since it tells us exactly what we want to know. + if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH) && fast_waccess(buf, X_OK) == 0) { return COMPLETE_EXEC_LINK_DESC; } @@ -403,10 +455,7 @@ static const wchar_t *file_get_desc(const wcstring &filename, int lstat_res, return COMPLETE_SOCKET_DESC; } else if (S_ISDIR(buf.st_mode)) { return COMPLETE_DIRECTORY_DESC; - } else if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH) && waccess(filename, X_OK) == 0) { - // Weird group permissions and other such issues make it non-trivial to find out if we can - // actually execute a file using the result from stat. It is much safer to use the access - // function, since it tells us exactly what we want to know. + } else if (buf.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH) && fast_waccess(buf, X_OK) == 0) { return COMPLETE_EXEC_DESC; } @@ -453,7 +502,7 @@ static bool wildcard_test_flags_then_complete(const wcstring &filepath, const wc } const bool executables_only = expand_flags & expand_flag::executables_only; - if (executables_only && (!is_executable || waccess(filepath, X_OK) != 0)) { + if (executables_only && (!is_executable || fast_waccess(stat_buf, X_OK) != 0)) { return false; } @@ -465,7 +514,7 @@ static bool wildcard_test_flags_then_complete(const wcstring &filepath, const wc // Compute the description. wcstring desc; if (!(expand_flags & expand_flag::no_descriptions)) { - desc = file_get_desc(filepath, lstat_res, lstat_buf, stat_res, stat_buf, stat_errno); + desc = file_get_desc(lstat_res, lstat_buf, stat_res, stat_buf, stat_errno); if (file_size >= 0) { if (!desc.empty()) desc.append(L", ");