From 1e3f415bed7c9384b4edb2aa8280cc0468d28b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Klaim=20=28Jo=C3=ABl=20Lamotte=29?= <142265+Klaim@users.noreply.github.com> Date: Mon, 3 Apr 2023 21:20:16 +0200 Subject: [PATCH] Windows: Fixed environment variables not read as unicode. (#2417) Windows: Fixed environment variables not read as unicode. --- libmamba/include/mamba/core/environment.hpp | 2 +- libmamba/include/mamba/core/util_os.hpp | 14 ++- libmamba/include/mamba/core/util_random.hpp | 1 + libmamba/src/core/environment.cpp | 99 ++++++++++++++------- libmamba/src/core/util_os.cpp | 42 +++++++++ libmamba/tests/CMakeLists.txt | 2 + libmamba/tests/src/core/test_system_env.cpp | 68 ++++++++++++++ libmamba/tests/src/core/test_util_os.cpp | 34 +++++++ 8 files changed, 224 insertions(+), 38 deletions(-) create mode 100644 libmamba/tests/src/core/test_system_env.cpp create mode 100644 libmamba/tests/src/core/test_util_os.cpp diff --git a/libmamba/include/mamba/core/environment.hpp b/libmamba/include/mamba/core/environment.hpp index ba10c47523..7b36abee94 100644 --- a/libmamba/include/mamba/core/environment.hpp +++ b/libmamba/include/mamba/core/environment.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2019, QuantStack and Mamba Contributors +// Copyright (c) 2019-2023, QuantStack and Mamba Contributors // // Distributed under the terms of the BSD 3-Clause License. // diff --git a/libmamba/include/mamba/core/util_os.hpp b/libmamba/include/mamba/core/util_os.hpp index e834b35308..f7e8576f02 100644 --- a/libmamba/include/mamba/core/util_os.hpp +++ b/libmamba/include/mamba/core/util_os.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2019, QuantStack and Mamba Contributors +// Copyright (c) 2019-2023, QuantStack and Mamba Contributors // // Distributed under the terms of the BSD 3-Clause License. // @@ -14,6 +14,11 @@ namespace mamba { +#ifdef _WIN32 + // Intention is to avoid including `Windows.h`, while still using the basic Windows API types. + using DWORD = unsigned long; +#endif + bool is_admin(); fs::u8path get_self_exe_path(); @@ -40,9 +45,10 @@ namespace mamba void reset_console(); #ifdef _WIN32 - std::string to_utf8(const wchar_t* w, size_t s); - std::string to_utf8(const wchar_t* w); - std::string to_utf8(const std::wstring& s); + std::string to_utf8(const wchar_t* windows_unicode_text, size_t size); + std::string to_utf8(const wchar_t* windows_unicode_text); + std::string to_utf8(const std::wstring& windows_unicode_text); + std::wstring to_windows_unicode(const std::string_view utf8_text); #endif /* Test whether a given `std::ostream` object refers to a terminal. */ diff --git a/libmamba/include/mamba/core/util_random.hpp b/libmamba/include/mamba/core/util_random.hpp index 8f23cfaaef..a761063ac6 100644 --- a/libmamba/include/mamba/core/util_random.hpp +++ b/libmamba/include/mamba/core/util_random.hpp @@ -8,6 +8,7 @@ #define MAMBA_CORE_UTIL_RANDOM_HPP #include +#include #include #include #include diff --git a/libmamba/src/core/environment.cpp b/libmamba/src/core/environment.cpp index bbd429ab50..7941cd6198 100644 --- a/libmamba/src/core/environment.cpp +++ b/libmamba/src/core/environment.cpp @@ -8,7 +8,10 @@ #include "mamba/core/util_string.hpp" #ifdef _WIN32 +#include + #include "mamba/core/output.hpp" +#include "mamba/core/util_os.hpp" #endif namespace mamba @@ -18,31 +21,54 @@ namespace mamba std::optional get(const std::string& key) { #ifdef _WIN32 - const size_t initial_size = 1024; - std::unique_ptr temp_small = std::make_unique(initial_size); - std::size_t size = GetEnvironmentVariableA(key.c_str(), temp_small.get(), initial_size); - if (size == 0) // Error or empty/missing + // See: + // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-s-wgetenv-s?view=msvc-170 + static std::mutex call_mutex; + std::scoped_lock ready_to_execute{ call_mutex }; // Calls to getenv_s kinds of + // functions are not thread-safe, this + // is to prevent related issues. + + const auto on_failed = [&](errno_t error_code) + { + LOG_ERROR << fmt::format( + "Failed to acquire environment variable '{}' : errcode = {}", + key, + error_code + ); + }; + + const std::wstring unicode_key = to_windows_unicode(key); + size_t required_size = 0; + if (auto error_code = _wgetenv_s(&required_size, nullptr, 0, unicode_key.c_str()); + error_code == 0) { - // Note that on Windows environment variables can never be empty, - // only missing. See https://stackoverflow.com/a/39095782 - auto last_err = GetLastError(); - if (last_err != ERROR_ENVVAR_NOT_FOUND) + if (required_size == 0) // The value doesn't exist. { - LOG_ERROR << "Could not get environment variable: " << last_err; + return {}; + } + + std::wstring value(required_size, L'?'); // Note: The required size implies a `\0` + // but basic_string doesn't. + if (error_code = _wgetenv_s( + &required_size, + value.data(), + value.size(), + unicode_key.c_str() + ); + error_code == 0) + { + value.pop_back(); // Remove the `\0` that was written in, otherwise any future + // concatenation will fail. + return mamba::to_utf8(value); + } + else + { + on_failed(error_code); } - return {}; - } - else if (size > initial_size) // Buffer too small - { - std::unique_ptr temp_large = std::make_unique(size); - GetEnvironmentVariableA(key.c_str(), temp_large.get(), size); - std::string res(temp_large.get()); - return res; } - else // Success + else { - std::string res(temp_small.get()); - return res; + on_failed(error_code); } #else const char* value = std::getenv(key.c_str()); @@ -50,22 +76,33 @@ namespace mamba { return value; } - else - { - return {}; - } #endif + return {}; } bool set(const std::string& key, const std::string& value) { #ifdef _WIN32 - auto res = SetEnvironmentVariableA(key.c_str(), value.c_str()); - if (!res) + // See: + // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-s-wgetenv-s?view=msvc-170 + static std::mutex call_mutex; + std::scoped_lock ready_to_execute{ call_mutex }; // Calls to getenv_s kinds of + // functions are not thread-safe, this + // is to prevent related issues. + + const std::wstring unicode_key = to_windows_unicode(key); + const std::wstring unicode_value = to_windows_unicode(value); + auto res = _wputenv_s(unicode_key.c_str(), unicode_value.c_str()); + if (res != 0) { - LOG_ERROR << "Could not set environment variable: " << GetLastError(); + LOG_ERROR << fmt::format( + "Could not set environment variable '{}' to '{}' : {}", + key, + value, + GetLastError() + ); } - return res; + return res == 0; #else return setenv(key.c_str(), value.c_str(), 1) == 0; #endif @@ -74,11 +111,7 @@ namespace mamba void unset(const std::string& key) { #ifdef _WIN32 - auto res = SetEnvironmentVariableA(key.c_str(), NULL); - if (!res) - { - LOG_ERROR << "Could not unset environment variable: " << GetLastError(); - } + set(key, ""); #else unsetenv(key.c_str()); #endif diff --git a/libmamba/src/core/util_os.cpp b/libmamba/src/core/util_os.cpp index c15ba674e1..00400ca21c 100644 --- a/libmamba/src/core/util_os.cpp +++ b/libmamba/src/core/util_os.cpp @@ -36,6 +36,10 @@ #include "mamba/core/util_os.hpp" #include "mamba/core/util_string.hpp" +#ifdef _WIN32 +static_assert(std::is_same_v); +#endif + namespace mamba { // Heavily inspired by https://github.com/gpakosz/whereami/ @@ -543,6 +547,44 @@ namespace mamba { return to_utf8(s.data(), s.size()); } + + std::wstring to_windows_unicode(const std::string_view utf8_text) + { + std::wstring output; + if (!utf8_text.empty()) + { + assert(utf8_text.size() <= INT_MAX); + const int size = MultiByteToWideChar( + CP_UTF8, + 0, + utf8_text.data(), + utf8_text.size(), + nullptr, + 0 + ); + if (size <= 0) + { + unsigned long last_error = ::GetLastError(); + LOG_ERROR << "Failed to convert UTF-8 string to Windows Unicode (UTF-16)" + << std::system_category().message(static_cast(last_error)); + throw std::runtime_error("Failed to convert UTF-8 string to UTF-16"); + } + + output.resize(size); + int res_size = MultiByteToWideChar( + CP_UTF8, + 0, + utf8_text.data(), + utf8_text.size(), + output.data(), + output.size() + ); + assert(res_size == size); + } + + return output; + } + #endif /* From https://github.com/ikalnytskyi/termcolor diff --git a/libmamba/tests/CMakeLists.txt b/libmamba/tests/CMakeLists.txt index 1640596305..04fe33407e 100644 --- a/libmamba/tests/CMakeLists.txt +++ b/libmamba/tests/CMakeLists.txt @@ -42,6 +42,8 @@ set(LIBMAMBA_TEST_SRCS src/core/test_virtual_packages.cpp src/core/test_util.cpp src/core/test_util_string.cpp + src/core/test_util_os.cpp + src/core/test_system_env.cpp src/core/test_env_lockfile.cpp src/core/test_execution.cpp src/core/test_invoke.cpp diff --git a/libmamba/tests/src/core/test_system_env.cpp b/libmamba/tests/src/core/test_system_env.cpp new file mode 100644 index 0000000000..12e5c07554 --- /dev/null +++ b/libmamba/tests/src/core/test_system_env.cpp @@ -0,0 +1,68 @@ +// Copyright (c) 2023, QuantStack and Mamba Contributors +// +// Distributed under the terms of the BSD 3-Clause License. +// +// The full license is in the file LICENSE, distributed with this software. + +#include + +#include + +#include "mamba/core/environment.hpp" +#include "mamba/core/util_random.hpp" + + +namespace +{ + void test_set_get_unset_env_variables(const std::string& key, const std::string& value) + { + const bool did_set = mamba::env::set(key, value); + ASSERT_TRUE(did_set); + + const auto result = mamba::env::get(key); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(*result, value); + + mamba::env::unset(key); + const auto result_empty = mamba::env::get(key); + ASSERT_FALSE(result_empty.has_value()); + } +} + +TEST(system_env, set_get_unset_env_variables) +{ + const auto key = mamba::generate_random_alphanumeric_string(128); + const auto value = mamba::generate_random_alphanumeric_string(128); + + test_set_get_unset_env_variables(key, value); +} + +TEST(system_env, set_get_unset_variables_unicode) +{ + const std::string key = u8"Joël私のにほん"; + const std::string value = u8"Hello, I am Joël. 私のにほんごわへたです"; + + test_set_get_unset_env_variables(key, value); +} + +#ifdef _WIN32 + +TEST(system_env, get_predefined_env_variable) +{ + // We check that normal pre-defined variables are accessible and do not fail access even if some + // of these values have unicode. + const std::vector predefined_keys{ "PATH", "OS", "PATHEXT", + "ProgramData", "SystemRoot", "windir", + "APPDATA", "COMPUTERNAME", "TEMP", + "UserName", "USERPROFILE" }; + + for (const auto& key : predefined_keys) + { + const auto maybe_value = mamba::env::get(key); + ASSERT_TRUE(maybe_value.has_value()) << "key = " + key; + const auto value = *maybe_value; + EXPECT_FALSE(value.empty()) << "key = " + key; + } +} + +#endif diff --git a/libmamba/tests/src/core/test_util_os.cpp b/libmamba/tests/src/core/test_util_os.cpp new file mode 100644 index 0000000000..4bc1f801f8 --- /dev/null +++ b/libmamba/tests/src/core/test_util_os.cpp @@ -0,0 +1,34 @@ +// Copyright (c) 2023, QuantStack and Mamba Contributors +// +// Distributed under the terms of the BSD 3-Clause License. +// +// The full license is in the file LICENSE, distributed with this software. + +#include + +#include + +#include "mamba/core/util_os.hpp" + + +#ifdef _WIN32 + +namespace +{ + const std::wstring text_utf16 = L"Hello, I am Joël. 私のにほんごわへたです"; + const std::string text_utf8 = u8"Hello, I am Joël. 私のにほんごわへたです"; +} + +TEST(to_utf8, basic_unicode_conversion) +{ + auto result = mamba::to_utf8(text_utf16); + EXPECT_EQ(text_utf8, result); +} + +TEST(to_windows_unicode, basic_unicode_conversion) +{ + auto result = mamba::to_windows_unicode(text_utf8); + EXPECT_EQ(text_utf16, result); +} + +#endif