From cf35431af9d9b09107a076de6ea9af4058ef29df Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sun, 21 Mar 2021 16:00:29 -0700
Subject: [PATCH] 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
---
 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 6a0c90740..9d7ffd092 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.
@@ -5369,6 +5379,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},
@@ -6251,6 +6268,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