refactor: tr_sys_path_resolve() returns a std::string (#3587)

* refactor: tr_sys_path_resolve returns std::string

CMakeLists.txt

@@ -566,7 +566,6 @@ endforeach()
 
 set(NEEDED_FUNCTIONS
     _configthreadlocale
-    canonicalize_file_name
     copy_file_range
     copyfile
     daemon

libtransmission/file-posix.cc

@@ -354,42 +354,25 @@ bool tr_sys_path_is_same(char const* path1, char const* path2, tr_error** error)
     return ret;
 }
 
-char* tr_sys_path_resolve(char const* path, tr_error** error)
+std::string tr_sys_path_resolve(std::string_view path, tr_error** error)
 {
-    TR_ASSERT(path != nullptr);
+    auto const szpath = tr_pathbuf{ path };
+    auto buf = std::array<char, PATH_MAX>{};
 
-    char* ret = nullptr;
+    if (auto* const ret = realpath(szpath, std::data(buf)); ret != nullptr)
-
-#if defined(HAVE_CANONICALIZE_FILE_NAME)
-
-    ret = canonicalize_file_name(path);
-
-#endif
-
-    if (ret == nullptr)
     {
-        char tmp[PATH_MAX];
+        return ret;
-        ret = realpath(path, tmp);
-
-        if (ret != nullptr)
-        {
-            ret = tr_strdup(ret);
-        }
     }
 
-    if (ret == nullptr)
+    set_system_error(error, errno);
-    {
+    return {};
-        set_system_error(error, errno);
-    }
-
-    return ret;
 }
 
 std::string tr_sys_path_basename(std::string_view path, tr_error** error)
 {
     auto tmp = tr_pathbuf{ path };
 
-    if (char const* ret = basename(std::data(tmp)); ret != nullptr)
+    if (char const* const ret = basename(std::data(tmp)); ret != nullptr)
     {
         return ret;
     }

libtransmission/file-win32.cc

@@ -39,6 +39,9 @@ struct tr_sys_dir_win32
     std::string utf8_name;
 };
 
+static auto constexpr NativeLocalPathPrefix = L"\\\\?\\"sv;
+static auto constexpr NativeUncPathPrefix = L"\\\\?\\UNC\\"sv;
+
 static wchar_t const native_local_path_prefix[] = { '\\', '\\', '?', '\\' };
 static wchar_t const native_unc_path_prefix[] = { '\\', '\\', '?', '\\', 'U', 'N', 'C', '\\' };
 
@@ -226,28 +229,28 @@ static std::wstring path_to_native_path_wstr(std::string_view path)
     return {};
 }
 
-static char* native_path_to_path(wchar_t const* wide_path)
+static std::string native_path_to_path(std::wstring_view wide_path)
 {
-    if (wide_path == nullptr)
+    if (std::empty(wide_path))
     {
-        return nullptr;
+        return {};
     }
 
-    bool const is_unc = wcsncmp(wide_path, native_unc_path_prefix, TR_N_ELEMENTS(native_unc_path_prefix)) == 0;
+    if (tr_strvStartsWith(wide_path, NativeUncPathPrefix))
-    bool const is_local = !is_unc && wcsncmp(wide_path, native_local_path_prefix, TR_N_ELEMENTS(native_local_path_prefix)) == 0;
-
-    size_t const skip_chars = is_unc ? TR_N_ELEMENTS(native_unc_path_prefix) :
-                                       (is_local ? TR_N_ELEMENTS(native_local_path_prefix) : 0);
-
-    char* const path = tr_win32_native_to_utf8_ex(wide_path + skip_chars, -1, is_unc ? 2 : 0, 0, nullptr);
-
-    if (is_unc && path != nullptr)
     {
-        path[0] = '\\';
+        wide_path.remove_prefix(std::size(NativeUncPathPrefix));
-        path[1] = '\\';
+        auto path = tr_win32_native_to_utf8(wide_path);
+        path.insert(0, "\\\\"sv);
+        return path;
     }
 
-    return path;
+    if (tr_strvStartsWith(wide_path, NativeLocalPathPrefix))
+    {
+        wide_path.remove_prefix(std::size(NativeLocalPathPrefix));
+        return tr_win32_native_to_utf8(wide_path);
+    }
+
+    return tr_win32_native_to_utf8(wide_path);
 }
 
 static tr_sys_file_t open_file(std::string_view path, DWORD access, DWORD disposition, DWORD flags, tr_error** error)
@@ -510,73 +513,46 @@ cleanup:
     return ret;
 }
 
-char* tr_sys_path_resolve(char const* path, tr_error** error)
+std::string tr_sys_path_resolve(std::string_view path, tr_error** error)
 {
-    TR_ASSERT(path != nullptr);
+    auto ret = std::string{};
 
-    char* ret = nullptr;
+    if (auto const wide_path = path_to_native_path_wstr(path); !std::empty(wide_path))
-    wchar_t* wide_ret = nullptr;
-    HANDLE handle = INVALID_HANDLE_VALUE;
-    DWORD wide_ret_size;
-
-    auto const wide_path = path_to_native_path_wstr(path);
-    if (std::empty(wide_path))
     {
-        goto fail;
+        if (auto const handle = CreateFileW(
+                wide_path.c_str(),
+                FILE_READ_EA,
+                FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                nullptr,
+                OPEN_EXISTING,
+                FILE_FLAG_BACKUP_SEMANTICS,
+                nullptr);
+            handle != INVALID_HANDLE_VALUE)
+        {
+            if (auto const wide_ret_size = GetFinalPathNameByHandleW(handle, nullptr, 0, 0); wide_ret_size != 0)
+            {
+                auto wide_ret = std::wstring{};
+                wide_ret.resize(wide_ret_size);
+                if (GetFinalPathNameByHandleW(handle, std::data(wide_ret), wide_ret_size, 0) == wide_ret_size - 1)
+                {
+                    // `wide_ret_size` includes the terminating '\0'; remove it from `wide_ret`
+                    wide_ret.resize(std::size(wide_ret) - 1);
+                    TR_ASSERT(tr_strvStartsWith(wide_ret, NativeLocalPathPrefix));
+                    ret = native_path_to_path(wide_ret);
+                }
+            }
+
+            CloseHandle(handle);
+        }
     }
 
-    handle = CreateFileW(
+    if (!std::empty(ret))
-        wide_path.c_str(),
-        FILE_READ_EA,
-        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-        nullptr,
-        OPEN_EXISTING,
-        FILE_FLAG_BACKUP_SEMANTICS,
-        nullptr);
-
-    if (handle == INVALID_HANDLE_VALUE)
     {
-        goto fail;
+        return ret;
     }
 
-    wide_ret_size = GetFinalPathNameByHandleW(handle, nullptr, 0, 0);
-
-    if (wide_ret_size == 0)
-    {
-        goto fail;
-    }
-
-    wide_ret = tr_new(wchar_t, wide_ret_size);
-
-    if (GetFinalPathNameByHandleW(handle, wide_ret, wide_ret_size, 0) != wide_ret_size - 1)
-    {
-        goto fail;
-    }
-
-    TR_ASSERT(wcsncmp(wide_ret, L"\\\\?\\", 4) == 0);
-
-    ret = native_path_to_path(wide_ret);
-
-    if (ret != nullptr)
-    {
-        goto cleanup;
-    }
-
-fail:
     set_system_error(error, GetLastError());
-
+    return {};
-    tr_free(ret);
-    ret = nullptr;
-
-cleanup:
-    tr_free(wide_ret);
-
-    if (handle != INVALID_HANDLE_VALUE)
-    {
-        CloseHandle(handle);
-    }
-
-    return ret;
 }
 
 std::string tr_sys_path_basename(std::string_view path, tr_error** error)
@@ -1347,6 +1323,8 @@ std::string tr_sys_dir_get_current(tr_error** error)
         wide_ret.resize(size);
         if (GetCurrentDirectoryW(std::size(wide_ret), std::data(wide_ret)) != 0)
         {
+            // `size` includes the terminating '\0'; remove it from `wide_ret`
+            wide_ret.resize(std::size(wide_ret) - 1);
             return tr_win32_native_to_utf8(wide_ret);
         }
     }

libtransmission/file.h

@@ -235,12 +235,10 @@ bool tr_sys_path_is_same(T const& path1, U const& path2, struct tr_error** error
  * @param[out] error Pointer to error object. Optional, pass `nullptr` if you
  *                   are not interested in error details.
  *
- * @return Pointer to newly allocated buffer containing full path (with symbolic
+ * @return Full path with symbolic links, `.` and `..` resolved on success,
- *         links, `.` and `..` resolved) on success (use @ref tr_free to free it
+ *         or an empty string otherwise (with `error` set accordingly).
- *         when no longer needed), `nullptr` otherwise (with `error` set
- *         accordingly).
  */
-char* tr_sys_path_resolve(char const* path, struct tr_error** error = nullptr);
+std::string tr_sys_path_resolve(std::string_view path, struct tr_error** error = nullptr);
 
 /**
  * @brief Portability wrapper for `basename()`.

libtransmission/utils.cc

@@ -192,26 +192,18 @@ bool tr_loadFile(std::string_view filename, std::vector<char>& contents, tr_erro
     return true;
 }
 
-bool tr_saveFile(std::string_view filename_in, std::string_view contents, tr_error** error)
+bool tr_saveFile(std::string_view filename, std::string_view contents, tr_error** error)
 {
-    auto const filename = tr_pathbuf{ filename_in };
     // follow symlinks to find the "real" file, to make sure the temporary
     // we build with tr_sys_file_open_temp() is created on the right partition
-    if (char* const real_filename = tr_sys_path_resolve(filename.c_str()); real_filename != nullptr)
+    if (auto const realname = tr_sys_path_resolve(filename); !std::empty(realname) && realname != filename)
     {
-        if (filename_in != real_filename)
+        return tr_saveFile(realname, contents, error);
-        {
-            auto const saved = tr_saveFile(real_filename, contents, error);
-            tr_free(real_filename);
-            return saved;
-        }
-
-        tr_free(real_filename);
     }
 
     // Write it to a temp file first.
     // This is a safeguard against edge cases, e.g. disk full, crash while writing, etc.
-    auto tmp = tr_pathbuf{ filename.sv(), ".tmp.XXXXXX"sv };
+    auto tmp = tr_pathbuf{ filename, ".tmp.XXXXXX"sv };
     auto const fd = tr_sys_file_open_temp(std::data(tmp), error);
     if (fd == TR_BAD_SYS_FILE)
     {
@@ -232,7 +224,8 @@ bool tr_saveFile(std::string_view filename_in, std::string_view contents, tr_err
     }
 
     // If we saved it to disk successfully, move it from '.tmp' to the correct filename
-    if (!tr_sys_file_close(fd, error) || !ok || !tr_sys_path_rename(tmp, filename, error))
+    auto const szfilename = tr_pathbuf{ filename };
+    if (!tr_sys_file_close(fd, error) || !ok || !tr_sys_path_rename(tmp, szfilename, error))
     {
         return false;
     }
@@ -301,7 +294,7 @@ std::string_view tr_strvStrip(std::string_view str)
 {
     auto constexpr test = [](auto ch)
     {
-        return isspace(ch);
+        return isspace(static_cast<unsigned char>(ch));
     };
 
     auto const it = std::find_if_not(std::begin(str), std::end(str), test);
@@ -519,78 +512,37 @@ std::string& tr_strvUtf8Clean(std::string_view cleanme, std::string& setme)
 
 #ifdef _WIN32
 
-std::string tr_win32_native_to_utf8(std::wstring_view native)
+std::string tr_win32_native_to_utf8(std::wstring_view in)
 {
-    if (auto* const tmp = tr_win32_native_to_utf8(std::data(native), std::size(native)); tmp != nullptr)
+    auto out = std::string{};
-    {
+    out.resize(WideCharToMultiByte(CP_UTF8, 0, std::data(in), std::size(in), nullptr, 0, nullptr, nullptr));
-        auto ret = std::string{ tmp };
+    auto len = WideCharToMultiByte(CP_UTF8, 0, std::data(in), std::size(in), std::data(out), std::size(out), nullptr, nullptr);
-        tr_free(tmp);
+    TR_ASSERT(len == std::size(out));
-        return ret;
+    return out;
-    }
-
-    return {};
-}
-
-std::wstring tr_win32_utf8_to_native(std::string_view utf8)
-{
-    if (auto* const tmp = tr_win32_utf8_to_native(std::data(utf8), std::size(utf8)); tmp != nullptr)
-    {
-        auto ret = std::wstring{ tmp };
-        tr_free(tmp);
-        return ret;
-    }
-
-    return {};
 }
 
 char* tr_win32_native_to_utf8(wchar_t const* text, int text_size)
 {
-    return tr_win32_native_to_utf8_ex(text, text_size, 0, 0, nullptr);
+    if (text == nullptr)
+    {
+        return nullptr;
+    }
+
+    if (text_size < 0)
+    {
+        return tr_strvDup(tr_win32_native_to_utf8(text));
+    }
+
+    return tr_strvDup(tr_win32_native_to_utf8({ text, static_cast<size_t>(text_size) }));
 }
 
-char* tr_win32_native_to_utf8_ex(
+std::wstring tr_win32_utf8_to_native(std::string_view in)
-    wchar_t const* text,
-    int text_size,
-    int extra_chars_before,
-    int extra_chars_after,
-    int* real_result_size)
 {
-    char* ret = nullptr;
+    auto out = std::wstring{};
-    int size;
+    out.resize(MultiByteToWideChar(CP_UTF8, 0, std::data(in), std::size(in), nullptr, 0));
-
+    auto len = MultiByteToWideChar(CP_UTF8, 0, std::data(in), std::size(in), std::data(out), std::size(out));
-    if (text_size == -1)
+    TR_ASSERT(len == std::size(out));
-    {
+    return out;
-        text_size = wcslen(text);
-    }
-
-    size = WideCharToMultiByte(CP_UTF8, 0, text, text_size, nullptr, 0, nullptr, nullptr);
-
-    if (size == 0)
-    {
-        goto fail;
-    }
-
-    ret = tr_new(char, size + extra_chars_before + extra_chars_after + 1);
-    size = WideCharToMultiByte(CP_UTF8, 0, text, text_size, ret + extra_chars_before, size, nullptr, nullptr);
-
-    if (size == 0)
-    {
-        goto fail;
-    }
-
-    ret[size + extra_chars_before + extra_chars_after] = '\0';
-
-    if (real_result_size != nullptr)
-    {
-        *real_result_size = size;
-    }
-
-    return ret;
-
-fail:
-    tr_free(ret);
-
-    return nullptr;
 }
 
 wchar_t* tr_win32_utf8_to_native(char const* text, int text_size)
@@ -1188,12 +1140,13 @@ std::string tr_env_get_string(std::string_view key, std::string_view default_val
     {
         if (auto const size = GetEnvironmentVariableW(wide_key.c_str(), nullptr, 0); size != 0)
         {
-            auto wide_val = std::vector<wchar_t>{};
+            auto wide_val = std::wstring{};
             wide_val.resize(size);
-
             if (GetEnvironmentVariableW(wide_key.c_str(), std::data(wide_val), std::size(wide_val)) == std::size(wide_val) - 1)
             {
-                return tr_win32_native_to_utf8({ std::data(wide_val), std::size(wide_val) });
+                TR_ASSERT(wide_val.back() == L'\0');
+                wide_val.resize(std::size(wide_val) - 1);
+                return tr_win32_native_to_utf8(wide_val);
             }
         }
     }

libtransmission/utils.h

@@ -123,12 +123,6 @@ std::string tr_win32_native_to_utf8(std::wstring_view);
 std::wstring tr_win32_utf8_to_native(std::string_view);
 
 char* tr_win32_native_to_utf8(wchar_t const* text, int text_size);
-char* tr_win32_native_to_utf8_ex(
-    wchar_t const* text,
-    int text_size,
-    int extra_chars_before,
-    int extra_chars_after,
-    int* real_result_size);
 wchar_t* tr_win32_utf8_to_native(char const* text, int text_size);
 wchar_t* tr_win32_utf8_to_native_ex(
     char const* text,
@@ -237,6 +231,11 @@ template<typename T>
     return std::size(key) <= std::size(sv) && sv.substr(0, std::size(key)) == key;
 }
 
+[[nodiscard]] constexpr bool tr_strvStartsWith(std::wstring_view sv, std::wstring_view key) // c++20
+{
+    return std::size(key) <= std::size(sv) && sv.substr(0, std::size(key)) == key;
+}
+
 [[nodiscard]] constexpr bool tr_strvEndsWith(std::string_view sv, std::string_view key) // c++20
 {
     return std::size(key) <= std::size(sv) && sv.substr(std::size(sv) - std::size(key)) == key;

libtransmission/variant-benc.cc

@@ -101,7 +101,7 @@ std::optional<std::string_view> ParseString(std::string_view* benc)
 
     // get the string length
     auto svtmp = benc->substr(0, colon_pos);
-    if (!std::all_of(std::begin(svtmp), std::end(svtmp), [](auto ch) { return isdigit(ch) != 0; }))
+    if (!std::all_of(std::begin(svtmp), std::end(svtmp), [](auto ch) { return isdigit(static_cast<unsigned char>(ch)) != 0; }))
     {
         return {};
     }

tests/libtransmission/file-test.cc

@@ -635,18 +635,18 @@ TEST_F(FileTest, pathResolve)
 
     if (createSymlink(path2, path1, false))
     {
-        auto tmp = makeString(tr_sys_path_resolve(path2, &err));
+        auto resolved = tr_sys_path_resolve(path2, &err);
         EXPECT_EQ(nullptr, err) << *err;
-        EXPECT_TRUE(pathContainsNoSymlinks(tmp.c_str()));
+        EXPECT_TRUE(pathContainsNoSymlinks(resolved.c_str()));
 
         tr_sys_path_remove(path2);
         tr_sys_path_remove(path1);
 
         tr_sys_dir_create(path1, 0, 0755);
         EXPECT_TRUE(createSymlink(path2, path1, true)); /* Win32: directory and file symlinks differ :( */
-        tmp = makeString(tr_sys_path_resolve(path2, &err));
+        resolved = tr_sys_path_resolve(path2, &err);
         EXPECT_EQ(nullptr, err) << *err;
-        EXPECT_TRUE(pathContainsNoSymlinks(tmp.c_str()));
+        EXPECT_TRUE(pathContainsNoSymlinks(resolved.c_str()));
     }
     else
     {
@@ -658,23 +658,25 @@ TEST_F(FileTest, pathResolve)
 
 #ifdef _WIN32
 
+    auto resolved = tr_sys_path_resolve("\\\\127.0.0.1\\NonExistent"sv, &err);
+    EXPECT_EQ(""sv, resolved);
+    EXPECT_NE(nullptr, err);
+    tr_error_clear(&err);
+
+    resolved = tr_sys_path_resolve("\\\\127.0.0.1\\ADMIN$\\NonExistent"sv, &err);
+    EXPECT_EQ(""sv, resolved);
+    EXPECT_NE(nullptr, err);
+    tr_error_clear(&err);
+
+    for (auto const& input : { "\\\\127.0.0.1\\ADMIN$\\System32"sv,
+                               "\\\\127.0.0.1\\ADMIN$\\\\System32"sv,
+                               "\\\\127.0.0.1\\\\ADMIN$\\System32"sv,
+                               "\\\\127.0.0.1\\\\ADMIN$\\\\System32"sv,
+                               "\\\\127.0.0.1\\ADMIN$/System32"sv })
     {
-        char* tmp;
+        resolved = tr_sys_path_resolve(input, &err);
-
+        EXPECT_EQ("\\\\127.0.0.1\\ADMIN$\\System32"sv, resolved);
-        tmp = tr_sys_path_resolve("\\\\127.0.0.1\\NonExistent", &err);
-        EXPECT_EQ(nullptr, tmp);
-        EXPECT_NE(nullptr, err);
-        tr_error_clear(&err);
-
-        tmp = tr_sys_path_resolve("\\\\127.0.0.1\\ADMIN$\\NonExistent", &err);
-        EXPECT_EQ(nullptr, tmp);
-        EXPECT_NE(nullptr, err);
-        tr_error_clear(&err);
-
-        tmp = tr_sys_path_resolve("\\\\127.0.0.1\\ADMIN$\\System32", &err);
-        EXPECT_STREQ("\\\\127.0.0.1\\ADMIN$\\System32", tmp);
         EXPECT_EQ(nullptr, err) << *err;
-        tr_free(tmp);
     }
 
 #endif

tests/libtransmission/subprocess-test.cc

@@ -31,7 +31,7 @@ namespace test
 
 std::string getTestProgramPath(std::string const& filename)
 {
-    auto const exe_path = makeString(tr_sys_path_resolve(testing::internal::GetArgvs().front().data()));
+    auto const exe_path = tr_sys_path_resolve(testing::internal::GetArgvs().front().data());
     auto const exe_dir = tr_sys_path_dirname(exe_path);
     return std::string{ exe_dir } + TR_PATH_DELIMITER + filename;
 }