From b2baf110c59ae8a990fdcdf78a7834c4c74e4295 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Sun, 21 Mar 2021 10:03:09 +0100 Subject: [PATCH 01/14] Disable pacman command-not-found handler Apparently it's too slow on some systems Fixes #7841. (cherry picked from commit 95dc821a448fac684c07f8a98fb3c7ed5b8445fa) --- share/functions/fish_command_not_found.fish | 23 +++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/share/functions/fish_command_not_found.fish b/share/functions/fish_command_not_found.fish index db568fd69..a6c43f920 100644 --- a/share/functions/fish_command_not_found.fish +++ b/share/functions/fish_command_not_found.fish @@ -57,17 +57,18 @@ else if type -q pkgfile __fish_default_command_not_found_handler $argv[1] end end -else if type -q pacman - function fish_command_not_found - set -l paths $argv[1] - # If we've not been given an absolute path, try $PATH as the starting point, - # otherwise pacman will try *every path*, and e.g. bash-completion - # isn't helpful. - string match -q '/*' -- $argv[1]; or set paths $PATH/$argv[1] - # Pacman only prints the path, so we still need to print the error. - __fish_default_command_not_found_handler $argv[1] - pacman -F $paths - end +# pacman is too slow, see #7841. +# else if type -q pacman +# function fish_command_not_found +# set -l paths $argv[1] +# # If we've not been given an absolute path, try $PATH as the starting point, +# # otherwise pacman will try *every path*, and e.g. bash-completion +# # isn't helpful. +# string match -q '/*' -- $argv[1]; or set paths $PATH/$argv[1] +# # Pacman only prints the path, so we still need to print the error. +# __fish_default_command_not_found_handler $argv[1] +# pacman -F $paths +# end else # Use standard fish command not found handler otherwise function fish_command_not_found --on-event fish_command_not_found From 9221a3decac8aa87106ee530232e710fe533d206 Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Sun, 21 Mar 2021 10:26:04 +0100 Subject: [PATCH 02/14] Only set modes after config.fish if we're *interactive* 013a563ed0fc2cb400d914efffbeb8b87061673c made it so we only try to adjust terminal modes if we are in the terminal pgroup, but that's not enough. Fish starts background jobs in events inside its own pgroup, so function on-foo --on-event foo fish -c 'sleep 3' & end would have the backgrounded fish try to fiddle with the terminal and succeed. Instead, only fiddle with the terminal if we're interactive (this should probably be extended to other bits, but this is the particular problematic part) Fixes #7842. (cherry picked from commit e4fd664bbb75d5abde9e5ffebdcb4118c58be6c3) --- src/reader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reader.cpp b/src/reader.cpp index b1167c2bc..25fcbf417 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1342,7 +1342,7 @@ void reader_init() { // Set up our fixed terminal modes once, // so we don't get flow control just because we inherited it. - if (getpgrp() == tcgetpgrp(STDIN_FILENO)) { + if (is_interactive_session()) { term_donate(/* quiet */ true); } From 0b144baa797760ca24116e432273a308709b676d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Mar 2021 12:39:17 -0700 Subject: [PATCH 03/14] Switch fish.pc dependency from FBVF file to CHECK-FBVF target Previously, both fish.pc and libfish had generating the FISH-BUILD-VERSION-FILE attached as a command. In principle they could both try to run the command simultaneously and now CMake complains about this with the Xcode generator. Switch to having fish.pc depend on the CHECK-FISH-BUILD-VERSION-FILE as a target instead of a command. This allows it to participate in dependency resolution and CMake will succeed again. Fixes #7838 (cherry picked from commit 1b950f5f3b2e5db88b3c2d694b5b25b0c284419c) --- cmake/Install.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake/Install.cmake b/cmake/Install.cmake index 5574b7894..af64029bd 100644 --- a/cmake/Install.cmake +++ b/cmake/Install.cmake @@ -110,7 +110,7 @@ add_custom_command(OUTPUT fish.pc COMMAND printf "Version: " >> fish.pc COMMAND sed 's/FISH_BUILD_VERSION=//\;s/\"//g' ${FBVF} >> fish.pc WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} - DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${FBVF} ${CMAKE_CURRENT_BINARY_DIR}/fish.pc.noversion) + DEPENDS CHECK-FISH-BUILD-VERSION-FILE ${CMAKE_CURRENT_BINARY_DIR}/fish.pc.noversion) add_custom_target(build_fish_pc ALL DEPENDS fish.pc) From 3a8e0e4c3773bfec975c6372f319067987056a02 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Mar 2021 14:50:37 -0700 Subject: [PATCH 04/14] Don't block certain error signals on background threads Previously fish attempted to block all signals on background threads, so that they would be delivered to the main thread. But on Mac, SIGSEGV and probably some others just get silently dropped, leading to potential infinite loops instead of crashing. So stop blocking these signals. With this change the null-deref in #7837 will properly crash instead of spinning. (cherry picked from commit a7c37e4af4b1c273a39a03fc994241313cf11e15) --- src/iothread.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/iothread.cpp b/src/iothread.cpp index f5434dda9..b2d7490e2 100644 --- a/src/iothread.cpp +++ b/src/iothread.cpp @@ -397,11 +397,18 @@ void iothread_perform_on_main(void_function_t &&func) { } bool make_detached_pthread(void *(*func)(void *), void *param) { - // The spawned thread inherits our signal mask. We don't want the thread to ever receive signals - // on the spawned thread, so temporarily block all signals, spawn the thread, and then restore - // it. + // The spawned thread inherits our signal mask. Temporarily block signals, spawn the thread, and + // then restore it. But we must not block SIGBUS, SIGFPE, SIGILL, or SIGSEGV; that's undefined + // (#7837). Conservatively don't try to mask SIGKILL or SIGSTOP either; that's ignored on Linux + // but maybe has an effect elsewhere. sigset_t new_set, saved_set; sigfillset(&new_set); + sigdelset(&new_set, SIGILL); // bad jump + sigdelset(&new_set, SIGFPE); // divide by zero + sigdelset(&new_set, SIGBUS); // unaligned memory access + sigdelset(&new_set, SIGSEGV); // bad memory access + sigdelset(&new_set, SIGSTOP); // unblockable + sigdelset(&new_set, SIGKILL); // unblockable DIE_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &new_set, &saved_set)); // Spawn a thread. If this fails, it means there's already a bunch of threads; it is very From 951fc6b95430aed244516f920f8f8c244663117a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Mar 2021 15:26:51 -0700 Subject: [PATCH 05/14] Add some tests for dirname and basename This is in preparation for replacing our wrappers around the C versions, with custom versions instead. (cherry picked from commit 6e1b32434300dd83c6addf44544897f08880cae3) --- src/fish_tests.cpp | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 079ee2601..e76a342c1 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -6184,6 +6184,42 @@ void test_normalize_path() { do_test(path_normalize_for_cd(L"/abc/def/", L"../ghi/..") == L"/abc/ghi/.."); } +void test_dirname_basename() { + say(L"Testing wdirname and wbasename"); + const struct testcase_t { + const wchar_t *path; + const wchar_t *dir; + const wchar_t *base; + } testcases[] = { + {L"", L".", L"."}, + {L"foo//", L".", L"foo"}, + {L"foo//////", L".", L"foo"}, + {L"/////foo", L"/", L"foo"}, + {L"/////foo", L"/", L"foo"}, + {L"//foo/////bar", L"//foo", L"bar"}, + {L"foo/////bar", L"foo", L"bar"}, + + // Examples given in XPG4.2. + {L"/usr/lib", L"/usr", L"lib"}, + {L"usr", L".", L"usr"}, + {L"/", L"/", L"/"}, + {L".", L".", L"."}, + {L"..", L".", L".."}, + }; + for (const auto &tc : testcases) { + wcstring dir = wdirname(tc.path); + if (dir != tc.dir) { + err(L"Wrong dirname for \"%ls\": expected \"%ls\", got \"%ls\"", tc.path, tc.dir, + dir.c_str()); + } + wcstring base = wbasename(tc.path); + if (base != tc.base) { + err(L"Wrong basename for \"%ls\": expected \"%ls\", got \"%ls\"", tc.path, tc.base, + base.c_str()); + } + } +} + static void test_topic_monitor() { say(L"Testing topic monitor"); topic_monitor_t monitor; @@ -6529,6 +6565,7 @@ int main(int argc, char **argv) { if (should_test_function("layout_cache")) test_layout_cache(); if (should_test_function("prompt")) test_prompt_truncation(); if (should_test_function("normalize")) test_normalize_path(); + if (should_test_function("dirname")) test_dirname_basename(); if (should_test_function("topics")) test_topic_monitor(); if (should_test_function("topics")) test_topic_monitor_torture(); if (should_test_function("pipes")) test_pipes(); From 8b825bf7608219497dd2e04110f764f1d5b62ede Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Mar 2021 16:00:29 -0700 Subject: [PATCH 06/14] Reimplement wbasename and wdirname Previously wbasename and wdirname wrapped the system-provided basename and dirname. But these have thread-safety issues and some surprising error conditions on Mac. Just reimplement these per the OpenGroup spec. In particular these no longer trigger a null-dereference if the input exceeds PATH_MAX. Add some tests too. This fixes #7837 (cherry picked from commit cf35431af9d9b09107a076de6ea9af4058ef29df) --- src/fish_tests.cpp | 22 ++++++++++++++++++++ src/wutil.cpp | 51 ++++++++++++++++++++++++++++++++++++++-------- src/wutil.h | 4 ++-- 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index e76a342c1..673430cfb 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -195,6 +195,16 @@ static void popd() { env_stack_t::principal().set_pwd_from_getcwd(); } +// Helper to return a string whose length greatly exceeds PATH_MAX. +wcstring get_overlong_path() { + wcstring longpath; + longpath.reserve(PATH_MAX * 2 + 10); + while (longpath.size() <= PATH_MAX * 2) { + longpath += L"/overlong"; + } + return longpath; +} + // The odd formulation of these macros is to avoid "multiple unary operator" warnings from oclint // were we to use the more natural "if (!(e)) err(..." form. We have to do this because the rules // for the C preprocessor make it practically impossible to embed a comment in the body of a macro. @@ -5336,6 +5346,13 @@ static void test_highlighting() { {L"# comment", highlight_role_t::comment}, }); + // Overlong paths don't crash (#7837). + const wcstring overlong = get_overlong_path(); + highlight_tests.push_back({ + {L"touch", highlight_role_t::command}, + {overlong.c_str(), highlight_role_t::param}, + }); + highlight_tests.push_back({ {L"a", highlight_role_t::param}, {L"=", highlight_role_t::operat, ns}, @@ -6218,6 +6235,11 @@ void test_dirname_basename() { base.c_str()); } } + // Ensures strings which greatly exceed PATH_MAX still work (#7837). + wcstring longpath = get_overlong_path(); + wcstring longpath_dir = longpath.substr(0, longpath.rfind(L'/')); + do_test(wdirname(longpath) == longpath_dir); + do_test(wbasename(longpath) == L"overlong"); } static void test_topic_monitor() { diff --git a/src/wutil.cpp b/src/wutil.cpp index dd515c3ca..d7478fe66 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -400,16 +400,51 @@ wcstring path_normalize_for_cd(const wcstring &wd, const wcstring &path) { return result; } -wcstring wdirname(const wcstring &path) { - std::string tmp = wcs2string(path); - const char *narrow_res = dirname(&tmp[0]); - return str2wcstring(narrow_res); +wcstring wdirname(wcstring path) { + // Do not use system-provided dirname (#7837). + // On Mac it's not thread safe, and will error for paths exceeding PATH_MAX. + // This follows OpenGroup dirname recipe. + // 1: Double-slash stays. + if (path == L"//") return path; + + // 2: All slashes => return slash. + if (!path.empty() && path.find_first_not_of(L'/') == wcstring::npos) return L"/"; + + // 3: Trim trailing slashes. + while (!path.empty() && path.back() == L'/') path.pop_back(); + + // 4: No slashes left => return period. + size_t last_slash = path.rfind(L'/'); + if (last_slash == wcstring::npos) return L"."; + + // 5: Remove trailing non-slashes. + path.erase(last_slash + 1, wcstring::npos); + + // 6: Skip as permitted. + // 7: Remove trailing slashes again. + while (!path.empty() && path.back() == L'/') path.pop_back(); + + // 8: Empty => return slash. + if (path.empty()) path = L"/"; + return path; } -wcstring wbasename(const wcstring &path) { - std::string tmp = wcs2string(path); - char *narrow_res = basename(&tmp[0]); - return str2wcstring(narrow_res); +wcstring wbasename(wcstring path) { + // This follows OpenGroup basename recipe. + // 1: empty => allowed to return ".". This is what system impls do. + if (path.empty()) return L"."; + + // 2: Skip as permitted. + // 3: All slashes => return slash. + if (!path.empty() && path.find_first_not_of(L'/') == wcstring::npos) return L"/"; + + // 4: Remove trailing slashes. + while (!path.empty() && path.back() == L'/') path.pop_back(); + + // 5: Remove up to and including last slash. + size_t last_slash = path.rfind(L'/'); + if (last_slash != wcstring::npos) path.erase(0, last_slash + 1); + return path; } // Really init wgettext. diff --git a/src/wutil.h b/src/wutil.h index 6ab8fb31a..555aa5266 100644 --- a/src/wutil.h +++ b/src/wutil.h @@ -75,10 +75,10 @@ bool wreaddir_resolving(DIR *dir, const std::wstring &dir_path, wcstring &out_na bool wreaddir_for_dirs(DIR *dir, wcstring *out_name); /// Wide character version of dirname(). -std::wstring wdirname(const std::wstring &path); +std::wstring wdirname(std::wstring path); /// Wide character version of basename(). -std::wstring wbasename(const std::wstring &path); +std::wstring wbasename(std::wstring path); /// Wide character wrapper around the gettext function. For historic reasons, unlike the real /// gettext function, wgettext takes care of setting the correct domain, etc. using the textdomain From e31096d7ed72659a89c08705c5ebe073bf99ae89 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 21 Mar 2021 16:51:09 -0700 Subject: [PATCH 07/14] Skip long arguments in syntax highlighting path detection When fish performs syntax highlighting, it attempts to determine which arguments are valid paths and underline them. Skip paths whose length exceeds PATH_MAX. This is an optimization: such strings are almost certainly not valid paths and checking them may be expensive. Relevant is #7837 (cherry picked from commit 8d54d2b60eae39ef7b5f5bc1a7ae83bc91703d95) --- src/highlight.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/highlight.cpp b/src/highlight.cpp index db07f3111..a1e432979 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -918,6 +918,11 @@ void highlighter_t::color_as_argument(const ast::node_t &node) { static bool range_is_potential_path(const wcstring &src, const source_range_t &range, const operation_context_t &ctx, const wcstring &working_directory) { + // Skip strings exceeding PATH_MAX. See #7837. + // Note some paths may exceed PATH_MAX, but this is just for highlighting. + if (range.length > PATH_MAX) { + return false; + } // Get the node source, unescape it, and then pass it to is_potential_path along with the // working directory (as a one element list). bool result = false; From 92c28291df1400a598919e9dc3094f9a742ab33b Mon Sep 17 00:00:00 2001 From: Fabian Homborg Date: Mon, 22 Mar 2021 16:57:55 +0100 Subject: [PATCH 08/14] Only donate term if we're interactive *and* have the terminal As it turns out otherwise fish would hang when sddm starts it as the login shell. Belongs to #7842. (cherry picked from commit 7f7cfcf339e27ed29b8f93abdbab4784621dd424) --- src/reader.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/reader.cpp b/src/reader.cpp index 25fcbf417..2ee73d5a7 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1342,7 +1342,8 @@ void reader_init() { // Set up our fixed terminal modes once, // so we don't get flow control just because we inherited it. - if (is_interactive_session()) { + if (is_interactive_session() && + getpgrp() == tcgetpgrp(STDIN_FILENO)) { term_donate(/* quiet */ true); } From 0efc55cbe942e0cb9346186ee96fb849c4be2459 Mon Sep 17 00:00:00 2001 From: Jannik Vieten Date: Tue, 23 Mar 2021 20:40:44 +0100 Subject: [PATCH 09/14] Fix completion errors for tshark when running as root (#7858) (cherry picked from commit 0f3274d5ebc7c7dc46b4b566ca32a061e7bebeb3) --- share/completions/tshark.fish | 6 +++--- share/functions/__fish_complete_wireshark.fish | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/share/completions/tshark.fish b/share/completions/tshark.fish index 1cb77495b..22827f22d 100644 --- a/share/completions/tshark.fish +++ b/share/completions/tshark.fish @@ -5,14 +5,14 @@ __fish_complete_wireshark tshark function __fish_tshark_protocols set -l tok (commandline -ct | string collect) set -l tok_param (string replace -r -- '^-O' '' $tok) - command tshark -G protocols | while read -l -d \t name shortname identifier + command tshark -G protocols 2>/dev/null | while read -l -d \t name shortname identifier printf "%s%s\t%s\n" (string replace -r -- '(.+),[^,]*$' '$1,' $tok_param) $tok_no_comma $identifier $name end end complete -c tshark -s 2 -d 'Perform a two-pass analysis' # This is fairly expensive, but only done upon the user pressing tab. -complete -c tshark -s e -d 'Add a field to the list of fields to display' -xa '(command tshark -G fields | awk -F\t \'{print $3"\t"$2}\')' +complete -c tshark -s e -d 'Add a field to the list of fields to display' -xa '(command tshark -G fields 2> /dev/null | awk -F\t \'{print $3"\t"$2}\')' complete -c tshark -s E -d 'Set an option controlling the printing of fields' -xa ' bom=y\t"Prepend output with the UTF-8 byte order mark" header=y\t"Print a list of the selected field names" @@ -24,7 +24,7 @@ quote=\t"Set the quote character to use to surround fields d=\", s=\', n=no quot complete -c tshark -s F -d 'Set the output capture file format' -xa '(command tshark -F 2>| string replace -rf "\s+(\S+) - (.*)" \'$1\t$2\')' complete -c tshark -s G -d 'Print a glossary' -xa '( printf "help\tList available report types\n" -command tshark -G help | string replace -rf "\s+-G (\S+)\s+(.*)" \'$1\t$2\' +command tshark -G help 2>/dev/null | string replace -rf "\s+-G (\S+)\s+(.*)" \'$1\t$2\' )' complete -c tshark -s H -d 'Read a list of entries from a "hosts" file' -r complete -c tshark -s j -d 'Protocol match filter used for ek|json|jsonraw|pdml output file types' -x diff --git a/share/functions/__fish_complete_wireshark.fish b/share/functions/__fish_complete_wireshark.fish index 4b82665d9..427f85afa 100644 --- a/share/functions/__fish_complete_wireshark.fish +++ b/share/functions/__fish_complete_wireshark.fish @@ -6,15 +6,15 @@ end function __fish_wireshark_interface # no remote capture yet - command tshark -D | string replace -r ".*\. (\S+)\s*\(?([^)]*)\)?\$" '$1\t$2' + command tshark -D 2>/dev/null | string replace -r ".*\. (\S+)\s*\(?([^)]*)\)?\$" '$1\t$2' end function __fish_wireshark_protocol - command tshark -G protocols | awk -F\t '{print $3"\t"$1}' + command tshark -G protocols 2>/dev/null | awk -F\t '{print $3"\t"$1}' end function __fish_wireshark_heuristic - command tshark -G heuristic-decodes | awk -F\t '{print $2"\t"$1}' + command tshark -G heuristic-decodes 2>/dev/null | awk -F\t '{print $2"\t"$1}' end function __fish_tshark_name_resolving_flags @@ -67,10 +67,10 @@ packets:\t"Switch to the next file after it contains N packets"' complete -c $shark -l list-time-stamp-types -d 'List time stamp types supported for the interface' complete -c $shark -s p -l no-promiscuous-mode -d "Don't put the interface into promiscuous mode" complete -c $shark -s s -l snapshot-length -d 'Set the default snapshot length in bytes to use when capturing live data' -x - complete -c $shark -l time-stamp-type -d "Change the interface's timestamp method" -xa '(__fish_wireshark_choices (command tshark --list-time-stamp-types))' + complete -c $shark -l time-stamp-type -d "Change the interface's timestamp method" -xa '(__fish_wireshark_choices (command tshark --list-time-stamp-types 2>/dev/null))' complete -c $shark -s v -l version -d 'Print the version and exit' complete -c $shark -s w -d 'Write raw packet data to the given file ("-" means stdout)' -r - complete -c $shark -s y -l linktype -d 'Set the data link type to use while capturing packets' -xa '(__fish_wireshark_choices (command tshark -L))' + complete -c $shark -s y -l linktype -d 'Set the data link type to use while capturing packets' -xa '(__fish_wireshark_choices (command tshark -L 2>/dev/null))' switch $shark case dumpcap tshark @@ -81,7 +81,7 @@ packets:\t"Switch to the next file after it contains N packets"' switch $shark case wireshark tshark complete -c $shark -s C -d 'Run with the given configuration profile' -xa '( -set -l folders (tshark -G folders | awk \'/Personal configuration/{ print $NF}\')/profiles/* +set -l folders (tshark -G folders 2>/dev/null | awk \'/Personal configuration/{ print $NF}\')/profiles/* string match -r "[^/]*\\$" -- $folders)' complete -c $shark -s d -d 'Specify how a layer type should be dissected' -xa '(__fish_tshark_decode_as)' complete -c $shark -l enable-protocol -d 'Enable dissection of the given protocol' -xa '(__fish_wireshark_protocol)' @@ -92,7 +92,7 @@ string match -r "[^/]*\\$" -- $folders)' complete -c $shark -s n -d 'Disable network object name resolution (hostname, TCP and UDP port names)' complete -c $shark -s N -d 'Turn on name resolution only for particular types of addresses and port numbers' -xa '( __fish_tshark_name_resolving_flags)' complete -c $shark -s o -d 'Override a preference value' -xa '( - command tshark -G defaultprefs | string replace -rf -- \'^#([a-z].*):.*\' \'$1:\')' + command tshark -G defaultprefs 2>/dev/null | string replace -rf -- \'^#([a-z].*):.*\' \'$1:\')' complete -c $shark -s r -l read-file -d 'Read packet data from the given file' -r complete -c $shark -s R -l read-filter -d 'Apply the given read filter' -x complete -c $shark -s t -d 'Set the format of the packet timestamp printed in summary lines' -xa ' From a4a42fa2c33c95c23e642348ac38be48eb8841a3 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Fri, 26 Mar 2021 07:46:35 +0100 Subject: [PATCH 10/14] completions/aura: remove outdated flag Commit a0b46e620 ("Update Aura completions") removed "abs", but forgot it here. Fixes #7865 (cherry picked from commit dc417f58ae55c888a98640929da63ff221098b7c) --- share/completions/aura.fish | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/share/completions/aura.fish b/share/completions/aura.fish index fb0ac40a7..ac607ebf8 100644 --- a/share/completions/aura.fish +++ b/share/completions/aura.fish @@ -87,7 +87,7 @@ for condition in query sync complete -c aura -n $$condition -s s -l search -r -d 'Search packages for regexp' end -for condition in abs aur +for condition in aur complete -c aura -n $$condition -s a -l delmakedeps -d 'Remove packages only needed during installation' complete -c aura -n $$condition -s d -l deps -d 'View package dependencies' complete -c aura -n $$condition -s i -l info -d 'View package information' From 797fbbb5f53500d7e7335da83f203a272720b874 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 3 Apr 2021 18:07:50 -0700 Subject: [PATCH 11/14] Remove the SIGIO signal handler and universal notifier If fish launches a program and that program marks stdin as O_ASYNC, then fish will start receiving SIGIO events on Mac. This occurs even though the file descriptor itself does not have the O_ASYNC flag set. SIGIO is reported as interrupting select which then breaks multiple-key bindings, especially in vi-mode. As the SIGIO based universal notifier is disabled, remove it and the SIGIO handler itself. This allows fish to ignore properly ignore SIGIO. Fixes #7853 --- src/env_universal_common.cpp | 126 ----------------------------------- src/env_universal_common.h | 3 - src/fish_tests.cpp | 12 +--- src/signal.cpp | 18 ----- src/signal.h | 4 -- 5 files changed, 1 insertion(+), 162 deletions(-) diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 2e2cbedb6..82791057e 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -1306,119 +1306,6 @@ static autoclose_fd_t make_fifo(const wchar_t *test_path, const wchar_t *suffix) return res; } -// A universal notifier which uses SIGIO with a named pipe. This relies on the following behavior -// which appears to work on Linux: if data becomes available on the pipe then all processes get -// SIGIO even if the data is read (i.e. there is no race between SIGIO and another proc reading). -// -// A key difference between Linux and Mac is that on Linux SIGIO is only delivered if the pipe was -// previously empty. That is, two successive writes with no reads will produce two SIGIOs on Mac but -// only one on Linux. -// -// So, to post a notification, write anything to the pipe; if that would block drain the pipe and -// try again. -// -// To receive a notification, watch for SIGIO on the read end, then read out the data to enable -// SIGIO to be delivered again. -class universal_notifier_sigio_t final : public universal_notifier_t { -#ifdef SIGIO - public: - // Note we use a different suffix from universal_notifier_named_pipe_t to produce different - // FIFOs. This is because universal_notifier_named_pipe_t is level triggered while we are edge - // triggered. If they shared the same FIFO, then the named_pipe variant would keep firing - // forever. - explicit universal_notifier_sigio_t(const wchar_t *test_path) - : pipe_(try_make_pipe(test_path, L".notify")) {} - - ~universal_notifier_sigio_t() = default; - - void post_notification() override { - if (!pipe_.valid()) return; - - // Write a byte to send SIGIO. If the pipe is full, read some and try again. - while (!write_1_byte()) { - if (errno == EINTR) { - continue; - } else if (errno == EAGAIN) { - // The pipe is full. Try once more. - drain_some(); - write_1_byte(); - break; - } else { - break; - } - } - } - - bool poll() override { - if (sigio_count_ != signal_get_sigio_count()) { - // On Mac, SIGIO is generated on every write to the pipe. - // On Linux, it is generated only when the pipe goes from empty to non-empty. - // Read from the pipe so that SIGIO may be delivered again. - drain_some(); - // We may have gotten another SIGIO because the pipe just became writable again. - // In particular BSD sends SIGIO on read even if there is no data to be read. - // Re-fetch the sigio count. - sigio_count_ = signal_get_sigio_count(); - return true; - } - return false; - } - - private: - // Construct a pipe for a given fifo path, arranging for SIGIO to be delivered. - // \return an invalid fd on failure. - static autoclose_fd_t try_make_pipe(const wchar_t *test_path, const wchar_t *suffix) { - autoclose_fd_t pipe = make_fifo(test_path, suffix); - if (!pipe.valid()) { - return autoclose_fd_t{}; - } - // Note that O_ASYNC cannot be passed to open() (see Linux kernel bug #5993). - // We have to set it afterwards like so. - // Linux got support for O_ASYNC on fifos in 2.6 (released 2003). Treat its absence as a - // failure, but don't be noisy if this fails. Non-Linux platforms without O_ASYNC should use - // a different notifier strategy to avoid running into this. -#ifdef O_ASYNC - if (fcntl(pipe.fd(), F_SETFL, O_NONBLOCK | O_ASYNC) == -1) -#endif - { - FLOGF(uvar_file, - _(L"fcntl(F_SETFL) failed, universal variable notifications disabled")); - return autoclose_fd_t{}; - } - if (fcntl(pipe.fd(), F_SETOWN, getpid()) == -1) { - FLOGF(uvar_file, - _(L"fcntl(F_SETOWN) failed, universal variable notifications disabled")); - return autoclose_fd_t{}; - } - return pipe; - } - - // Try writing a single byte to our pipe. - // \return true on success, false on error, in which case errno will be set. - bool write_1_byte() const { - assert(pipe_.valid() && "Invalid pipe"); - char c = 0x42; - ssize_t amt = write(pipe_.fd(), &c, sizeof c); - return amt > 0; - } - - // Read some data off of the pipe, discarding it. - void drain_some() const { - if (!pipe_.valid()) return; - char buff[256]; - ignore_result(read(pipe_.fd(), buff, sizeof buff)); - } - - autoclose_fd_t pipe_{}; - uint32_t sigio_count_{0}; -#else - public: - [[noreturn]] universal_notifier_sigio_t() { - DIE("universal_notifier_sigio_t cannot be used on this system"); - } -#endif -}; - // Named-pipe based notifier. All clients open the same named pipe for reading and writing. The // pipe's readability status is a trigger to enter polling mode. // @@ -1582,16 +1469,6 @@ universal_notifier_t::notifier_strategy_t universal_notifier_t::resolve_default_ return strategy_notifyd; #elif defined(__CYGWIN__) return strategy_shmem_polling; -#elif 0 && defined(SIGIO) && (defined(__APPLE__) || defined(__BSD__) || defined(__linux__)) - // FIXME: The SIGIO notifier relies on an extremely specific interaction between signal handling and - // O_ASYNC writes, and doesn't currently work particularly well, so it's disabled. - // See discussion in #6585 and #7774 for examples of breakage. - // - // The SIGIO notifier does not yet work on WSL. See #7429 - if (is_windows_subsystem_for_linux()) { - return strategy_named_pipe; - } - return strategy_sigio; #else return strategy_named_pipe; #endif @@ -1612,9 +1489,6 @@ std::unique_ptr universal_notifier_t::new_notifier_for_str case strategy_shmem_polling: { return make_unique(); } - case strategy_sigio: { - return make_unique(test_path); - } case strategy_named_pipe: { return make_unique(test_path); } diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 2d852a54d..45cb9bbd9 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -166,9 +166,6 @@ class universal_notifier_t { // Mac-specific notify(3) implementation. strategy_notifyd, - // Set up a fifo and then waits for SIGIO to be delivered on it. - strategy_sigio, - // Strategy that uses a named pipe. Somewhat complex, but portable and doesn't require // polling most of the time. strategy_named_pipe, diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 673430cfb..7b86ce338 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -3987,8 +3987,7 @@ static void trigger_or_wait_for_notification(universal_notifier_t::notifier_stra usleep(40000); break; } - case universal_notifier_t::strategy_named_pipe: - case universal_notifier_t::strategy_sigio: { + case universal_notifier_t::strategy_named_pipe: { break; // nothing required } } @@ -3999,9 +3998,6 @@ static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy std::unique_ptr notifiers[16]; size_t notifier_count = sizeof notifiers / sizeof *notifiers; - // Set up SIGIO handler as needed. - signal_set_handlers(false); - // Populate array of notifiers. for (size_t i = 0; i < notifier_count; i++) { notifiers[i] = universal_notifier_t::new_notifier_for_strategy(strategy, UVARS_TEST_PATH); @@ -4051,12 +4047,6 @@ static void test_notifiers_with_strategy(universal_notifier_t::notifier_strategy // Nobody should poll now. for (size_t i = 0; i < notifier_count; i++) { - // On BSD, SIGIO may be delivered by read() even if it returns EAGAIN; - // that is the polling itself may trigger a SIGIO. Therefore we poll twice. - if (strategy == universal_notifier_t::strategy_sigio) { - (void)poll_notifier(notifiers[i]); - } - if (poll_notifier(notifiers[i])) { err(L"Universal variable notifier polled true after all changes, with strategy %d", (int)strategy); diff --git a/src/signal.cpp b/src/signal.cpp index c763b861d..16fe044ea 100644 --- a/src/signal.cpp +++ b/src/signal.cpp @@ -207,11 +207,6 @@ void signal_clear_cancel() { s_cancellation_signal = 0; } int signal_check_cancel() { return s_cancellation_signal; } -/// Number of SIGIO events. -static volatile relaxed_atomic_t s_sigio_count{0}; - -uint32_t signal_get_sigio_count() { return s_sigio_count; } - /// The single signal handler. By centralizing signal handling we ensure that we can never install /// the "wrong" signal handler (see #5969). static void fish_signal_handler(int sig, siginfo_t *info, void *context) { @@ -276,14 +271,6 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) { // test, to verify that we behave correctly when receiving lots of irrelevant signals. break; -#if defined(SIGIO) - case SIGIO: - // An async FD became readable/writable/etc. - // Don't try to look at si_code, it is not set under BSD. - // Don't use ++ to avoid a CAS. - s_sigio_count = s_sigio_count + 1; - break; -#endif } errno = saved_errno; } @@ -366,11 +353,6 @@ void signal_set_handlers(bool interactive) { act.sa_flags = SA_SIGINFO; sigaction(SIGINT, &act, nullptr); - // Apply our SIGIO handler. - act.sa_sigaction = &fish_signal_handler; - act.sa_flags = SA_SIGINFO; - sigaction(SIGIO, &act, nullptr); - // Whether or not we're interactive we want SIGCHLD to not interrupt restartable syscalls. act.sa_sigaction = &fish_signal_handler; act.sa_flags = SA_SIGINFO | SA_RESTART; diff --git a/src/signal.h b/src/signal.h index 3889ef806..99c318646 100644 --- a/src/signal.h +++ b/src/signal.h @@ -42,10 +42,6 @@ int signal_check_cancel(); /// In generaly this should only be done in interactive sessions. void signal_clear_cancel(); -/// \return a count of SIGIO signals. -/// This is used by universal variables, and is a simple unsigned counter which wraps to 0. -uint32_t signal_get_sigio_count(); - enum class topic_t : uint8_t; /// A sigint_detector_t can be used to check if a SIGINT (or SIGHUP) has been delivered. class sigchecker_t { From fa890dc2336ebb02d30051de74a02ca0fdc3b5a6 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 5 Apr 2021 13:04:05 -0700 Subject: [PATCH 12/14] Prevent hanging when restoring the foreground process group at exit When fish starts, it notices which pgroup owns the tty, and then it restores that pgroup's tty ownership when it exits. However if fish does not own the tty, then (on Mac at least) the tcsetpgrp call triggers a SIGSTOP and fish will hang while trying to exit. The first change is to ignore SIGTTOU instead of defaulting it. This prevents the hang; however it risks re-introducing #7060. The second change somewhat mitigates the risk of the first: only do the restore if the initial pgroup is different than fish's pgroup. This prevents some useless calls which might potentially steal the tty from another process (e.g. in #7060). --- src/common.cpp | 15 ++++--- src/fish_test_helper.cpp | 21 +++++++++ tests/pexpects/exit_nohang.py | 82 +++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 tests/pexpects/exit_nohang.py diff --git a/src/common.cpp b/src/common.cpp index a1b3125cf..06cf735d9 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -1789,12 +1789,15 @@ void save_term_foreground_process_group() { } void restore_term_foreground_process_group_for_exit() { - if (initial_fg_process_group != -1) { - // This is called during shutdown and from a signal handler. We don't bother to complain on - // failure because doing so is unlikely to be noticed. - // However we want this to fail if we are not the tty owner (#7060), so clear our SIGTTOU - // handler to allow it to fail properly. Note that we are about to exit. - (void)signal(SIGTTOU, SIG_DFL); + // We wish to restore the tty to the initial owner. There's two ways this can go wrong: + // 1. We may steal the tty from someone else (#7060). + // 2. The call to tcsetpgrp may deliver SIGSTOP to us, and we will not exit. + // Hanging on exit seems worse, so ensure that SIGTTOU is ignored so we do not get SIGSTOP. + // Note initial_fg_process_group == 0 is possible with Linux pid namespaces. + // This is called during shutdown and from a signal handler. We don't bother to complain on + // failure because doing so is unlikely to be noticed. + if (initial_fg_process_group > 0 && initial_fg_process_group != getpgrp()) { + (void)signal(SIGTTOU, SIG_IGN); (void)tcsetpgrp(STDIN_FILENO, initial_fg_process_group); } } diff --git a/src/fish_test_helper.cpp b/src/fish_test_helper.cpp index 1270772db..94575754d 100644 --- a/src/fish_test_helper.cpp +++ b/src/fish_test_helper.cpp @@ -20,6 +20,26 @@ static void become_foreground_then_print_stderr() { fprintf(stderr, "become_foreground_then_print_stderr done\n"); } +static void nohup_wait() { + pid_t init_parent = getppid(); + if (signal(SIGHUP, SIG_IGN)) { + perror("tcsetgrp"); + exit(EXIT_FAILURE); + } + // Note: these silly close() calls are necessary to prevent our parent process (presumably fish) + // from getting stuck in the "E" state ("Trying to exit"). This appears to be a (kernel?) bug on + // macOS: the process is no longer running but is not a zombie either, and so cannot be reaped. + // It is unclear why closing these fds successfully works around this issue. + close(STDIN_FILENO); + close(STDOUT_FILENO); + close(STDERR_FILENO); + // To avoid leaving fish_test_helpers around, we exit once our parent changes, meaning the fish + // instance exited. + while (getppid() == init_parent) { + usleep(1000000 / 4); + } +} + static void report_foreground_loop() { int was_fg = -1; const auto grp = getpgrp(); @@ -146,6 +166,7 @@ struct fth_command_t { static fth_command_t s_commands[] = { {"become_foreground_then_print_stderr", become_foreground_then_print_stderr, "Claim the terminal (tcsetpgrp) and then print to stderr"}, + {"nohup_wait", nohup_wait, "Ignore SIGHUP and just wait"}, {"report_foreground", report_foreground, "Report to stderr whether we own the terminal"}, {"report_foreground_loop", report_foreground_loop, "Continually report to stderr whether we own the terminal"}, diff --git a/tests/pexpects/exit_nohang.py b/tests/pexpects/exit_nohang.py new file mode 100644 index 000000000..9c4a81df0 --- /dev/null +++ b/tests/pexpects/exit_nohang.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python3 +from pexpect_helper import SpawnedProc +import subprocess +import sys +import signal +import time +import os + +sp = SpawnedProc() +send, sendline, sleep, expect_prompt, expect_re = ( + sp.send, + sp.sendline, + sp.sleep, + sp.expect_prompt, + sp.expect_re, +) + +# Helper to print an error and exit. +def error_and_exit(text): + keys = sp.colors() + print("{RED}Test failed: {NORMAL}{TEXT}".format(TEXT=text, **keys)) + sys.exit(1) + + +fish_pid = sp.spawn.pid + +# Launch fish_test_helper. +expect_prompt() +exe_path = os.environ.get("fish_test_helper") +sp.sendline(exe_path + " nohup_wait") + +# We expect it to transfer tty ownership to fish_test_helper. +sleep(0.1) +tty_owner = os.tcgetpgrp(sp.spawn.child_fd) +if fish_pid == tty_owner: + os.kill(fish_pid, signal.SIGKILL) + error_and_exit( + "Test failed: outer fish %d did not transfer tty owner to fish_test_helper" + % (fish_pid) + ) + + +# Now we are going to tell fish to exit. +# It must not hang. But it might hang when trying to restore the tty. +os.kill(fish_pid, signal.SIGTERM) + +# Loop a bit until the process exits (correct) or stops (incorrrect). +# When it exits it should be due to the SIGTERM that we sent it. +for i in range(10): + pid, status = os.waitpid(fish_pid, os.WUNTRACED | os.WNOHANG) + if pid == 0: + # No process ready yet, loop again. + sleep(0.1) + elif pid != fish_pid: + # Some other process exited (??) + os.kill(fish_pid, signal.SIGKILL) + error_and_exit( + "unexpected pid returned from waitpid. Got %d, expected %d" + % (pid, fish_pid) + ) + elif os.WIFSTOPPED(status): + # Our pid stopped instead of exiting. + # Probably it stopped because of its tcsetpgrp call during exit. + # Kill it and report a failure. + os.kill(fish_pid, signal.SIGKILL) + error_and_exit("pid %d stopped instead of exiting on SIGTERM" % pid) + elif not os.WIFSIGNALED(status): + error_and_exit("pid %d did not signal-exit" % pid) + elif os.WTERMSIG(status) != signal.SIGTERM: + error_and_exit( + "pid %d signal-exited with %d instead of %d (SIGTERM)" + % (os.WTERMSIG(status), signal.SIGTERM) + ) + else: + # Success! + sys.exit(0) +else: + # Our loop completed without the process being returned. + error_and_exit("fish with pid %d hung after SIGTERM" % fish_pid) + +# Should never get here. +error_and_exit("unknown, should be unreachable") From da0c9da880a1e66c8807518f6c6e336ffff29707 Mon Sep 17 00:00:00 2001 From: David Adam Date: Tue, 6 Apr 2021 23:32:51 +0800 Subject: [PATCH 13/14] CHANGELOG: work on 3.2.2 --- CHANGELOG.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 43be13e5b..7e6e209b9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,20 @@ +fish 3.2.2 (released April XX, 2021) +==================================== + +This release of fish fixes a number of additional issues identified in the fish 3.2 series: + +- The command-not-found handler used suggestions from ``pacman`` on Arch Linux, but this caused major slowdowns on some systems and has been disabled (:issue:`7841`). +- fish will no longer hang on exit if another process is in the foreground on macOS (:issue:`7901`). +- Certain programs (such as ``lazygit``) could create situations where fish would not receive keystrokes correctly, but it is now more robust in these situations (:issue:`7853`). +- Arguments longer than 1024 characters no longer trigger excessive CPU usage on macOS (:issue:`7837`). +- fish builds correctly on macOS when using new versions of Xcode (:issue:`7838`). +- Completions for ``aura`` (:issue:`7865`) and ``tshark`` (:issue:`7858`) should no longer produce errors. +- Background jobs no longer interfere with syntax highlighting (a regression introduced in fish 3.2.1, :issue:`7842`). + +If you are upgrading from version 3.1.2 or before, please also review the release notes for 3.2.1 and 3.2.0 (included below). + +-------------- + fish 3.2.1 (released March 18, 2021) ==================================== From 0c2cbfc01f686f11d0015c36ba66e7144794f537 Mon Sep 17 00:00:00 2001 From: David Adam Date: Wed, 7 Apr 2021 20:31:43 +0800 Subject: [PATCH 14/14] Release 3.2.2 Closes #7889. --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7e6e209b9..2f111e1ab 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,4 +1,4 @@ -fish 3.2.2 (released April XX, 2021) +fish 3.2.2 (released April 7, 2021) ==================================== This release of fish fixes a number of additional issues identified in the fish 3.2 series: