diff --git a/libtransmission/subprocess-posix.cc b/libtransmission/subprocess-posix.cc index 0d816249c..b227e72a0 100644 --- a/libtransmission/subprocess-posix.cc +++ b/libtransmission/subprocess-posix.cc @@ -52,16 +52,23 @@ static void set_system_error(tr_error** error, int code, char const* what) } } -static bool tr_spawn_async_in_child(char* const* cmd, char* const* env, char const* work_dir, int pipe_fd) +static bool tr_spawn_async_in_child( + char const* const* cmd, + std::map const& env, + char const* work_dir, + int pipe_fd) { - if (env != nullptr) + auto key_sz = std::string{}; + auto val_sz = std::string{}; + + for (auto const& [key_sv, val_sv] : env) { - for (size_t i = 0; env[i] != nullptr; ++i) + key_sz = key_sv; + val_sz = val_sv; + + if (setenv(key_sz.c_str(), val_sz.c_str(), 1) != 0) { - if (putenv(env[i]) != 0) - { - goto FAIL; - } + goto FAIL; } } @@ -70,7 +77,7 @@ static bool tr_spawn_async_in_child(char* const* cmd, char* const* env, char con goto FAIL; } - if (execvp(cmd[0], cmd) == -1) + if (execvp(cmd[0], const_cast(cmd)) == -1) { goto FAIL; } @@ -115,7 +122,11 @@ static bool tr_spawn_async_in_parent(int pipe_fd, tr_error** error) return true; } -bool tr_spawn_async(char* const* cmd, char* const* env, char const* work_dir, tr_error** error) +bool tr_spawn_async( + char const* const* cmd, + std::map const& env, + char const* work_dir, + tr_error** error) { static bool sigchld_handler_set = false; diff --git a/libtransmission/subprocess-win32.cc b/libtransmission/subprocess-win32.cc index 2fde53323..a2dfc41ff 100644 --- a/libtransmission/subprocess-win32.cc +++ b/libtransmission/subprocess-win32.cc @@ -20,6 +20,8 @@ #include "tr-assert.h" #include "utils.h" +using namespace std::literals; + enum tr_app_type { TR_APP_TYPE_EXE, @@ -128,28 +130,19 @@ static int compare_env_part_names(void const* vlhs, void const* vrhs) return ret; } -static wchar_t** to_wide_env(char const* const* env) +static wchar_t** to_wide_env(std::map const& env) { - if (env == nullptr || env[0] == nullptr) + auto const part_count = std::size(env); + wchar_t** const wide_env = tr_new(wchar_t*, part_count + 1); + + int i = 0; + for (auto const& [key_sv, val_sv] : env) { - return nullptr; + auto const line = tr_strvJoin(key_sv, "="sv, val_sv); + wide_env[i++] = tr_win32_utf8_to_native(std::data(line), std::size(line)); } - - size_t part_count = 0; - - while (env[part_count] != nullptr) - { - ++part_count; - } - - wchar_t** wide_env = tr_new(wchar_t*, part_count + 1); - - for (size_t i = 0; i < part_count; ++i) - { - wide_env[i] = tr_win32_utf8_to_native(env[i], -1); - } - - wide_env[part_count] = nullptr; + wide_env[i] = nullptr; + TR_ASSERT(i == part_count); /* "The sort is case-insensitive, Unicode order, without regard to locale" (c) MSDN */ qsort(wide_env, part_count, sizeof(wchar_t*), &compare_env_part_names); @@ -157,7 +150,7 @@ static wchar_t** to_wide_env(char const* const* env) return wide_env; } -static bool create_env_block(char const* const* env, wchar_t** env_block, tr_error** error) +static bool create_env_block(std::map const& env, wchar_t** env_block, tr_error** error) { wchar_t** wide_env = to_wide_env(env); @@ -379,7 +372,11 @@ cleanup: return ret; } -bool tr_spawn_async(char* const* cmd, char* const* env, char const* work_dir, tr_error** error) +bool tr_spawn_async( + char const* const* cmd, + std::map const& env, + char const* work_dir, + tr_error** error) { wchar_t* env_block = nullptr; diff --git a/libtransmission/subprocess.h b/libtransmission/subprocess.h index b2a0bfab4..fe956b482 100644 --- a/libtransmission/subprocess.h +++ b/libtransmission/subprocess.h @@ -8,6 +8,13 @@ #pragma once +#include +#include + #include "tr-macros.h" -bool tr_spawn_async(char* const* cmd, char* const* env, char const* work_dir, struct tr_error** error); +bool tr_spawn_async( + char const* const* cmd, + std::map const& env, + char const* work_dir, + struct tr_error** error); diff --git a/libtransmission/torrent.cc b/libtransmission/torrent.cc index ebeca4789..78e2fc5a8 100644 --- a/libtransmission/torrent.cc +++ b/libtransmission/torrent.cc @@ -1985,21 +1985,20 @@ static void torrentCallScript(tr_torrent const* tor, char const* script) char* const torrent_dir = tr_sys_path_native_separators(tr_strdup(tor->currentDir)); - char* const cmd[] = { - tr_strdup(script), + char const* const cmd[] = { + script, nullptr, }; - char* const env[] = { - tr_strdup_printf("TR_APP_VERSION=%s", SHORT_VERSION_STRING), - tr_strdup_printf("TR_TIME_LOCALTIME=%s", ctime_str), - tr_strdup_printf("TR_TORRENT_DIR=%s", torrent_dir), - tr_strdup_printf("TR_TORRENT_HASH=%s", tor->info.hashString), - tr_strdup_printf("TR_TORRENT_ID=%d", tr_torrentId(tor)), - tr_strdup_printf("TR_TORRENT_LABELS=%s", buildLabelsString(tor).c_str()), - tr_strdup_printf("TR_TORRENT_NAME=%s", tr_torrentName(tor)), - tr_strdup_printf("TR_TORRENT_TRACKERS=%s", buildTrackersString(tor).c_str()), - nullptr, + auto const env = std::map{ + { "TR_APP_VERSION"sv, SHORT_VERSION_STRING }, + { "TR_TIME_LOCALTIME"sv, ctime_str }, + { "TR_TORRENT_DIR"sv, torrent_dir }, + { "TR_TORRENT_HASH"sv, tor->info.hashString }, + { "TR_TORRENT_ID"sv, std::to_string(tr_torrentId(tor)) }, + { "TR_TORRENT_LABELS"sv, buildLabelsString(tor) }, + { "TR_TORRENT_NAME"sv, tr_torrentName(tor) }, + { "TR_TORRENT_TRACKERS"sv, buildTrackersString(tor) }, }; tr_logAddTorInfo(tor, "Calling script \"%s\"", script); @@ -2012,8 +2011,6 @@ static void torrentCallScript(tr_torrent const* tor, char const* script) tr_error_free(error); } - tr_free_ptrv((void* const*)env); - tr_free_ptrv((void* const*)cmd); tr_free(torrent_dir); } diff --git a/tests/libtransmission/subprocess-test.cc b/tests/libtransmission/subprocess-test.cc index 80d02a7e7..87dfc816d 100644 --- a/tests/libtransmission/subprocess-test.cc +++ b/tests/libtransmission/subprocess-test.cc @@ -87,14 +87,10 @@ TEST_P(SubprocessTest, SpawnAsyncMissingExec) { auto const missing_exe_path = std::string{ TR_IF_WIN32("C:\\", "/") "tr-missing-test-exe" TR_IF_WIN32(".exe", "") }; - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(missing_exe_path.data()), - nullptr - }; + auto args = std::array{ missing_exe_path.data(), nullptr }; tr_error* error = nullptr; - auto const ret = tr_spawn_async(args.data(), nullptr, nullptr, &error); + auto const ret = tr_spawn_async(std::data(args), {}, nullptr, &error); EXPECT_FALSE(ret); EXPECT_NE(nullptr, error); EXPECT_NE(0, error->code); @@ -113,20 +109,17 @@ TEST_P(SubprocessTest, SpawnAsyncArgs) auto const test_arg3 = std::string{}; auto const test_arg4 = std::string{ "\"arg3'^! $PATH %PATH% \\" }; - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(self_path_.c_str()), - tr_strdup(result_path.data()), - tr_strdup(arg_dump_args_.data()), - tr_strdup(test_arg1.data()), - tr_strdup(test_arg2.data()), - tr_strdup(test_arg3.data()), - tr_strdup(allow_batch_metachars ? test_arg4.data() : nullptr), - nullptr - }; + auto const args = std::array{ self_path_.c_str(), + result_path.data(), + arg_dump_args_.data(), + test_arg1.data(), + test_arg2.data(), + test_arg3.data(), + allow_batch_metachars ? test_arg4.data() : nullptr, + nullptr }; tr_error* error = nullptr; - bool const ret = tr_spawn_async(args.data(), nullptr, nullptr, &error); + bool const ret = tr_spawn_async(std::data(args), {}, nullptr, &error); EXPECT_TRUE(ret) << args[0] << ' ' << args[1]; EXPECT_EQ(nullptr, error) << error->code << ", " << error->message; @@ -183,34 +176,31 @@ TEST_P(SubprocessTest, SpawnAsyncEnv) auto const test_env_value4 = std::string{ "bar" }; auto const test_env_value5 = std::string{ "jar" }; - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(self_path_.c_str()), // - tr_strdup(result_path.data()), // - tr_strdup(arg_dump_env_.data()), // - tr_strdup(test_env_key1.data()), // - tr_strdup(test_env_key2.data()), // - tr_strdup(test_env_key3.data()), // - tr_strdup(test_env_key4.data()), // - tr_strdup(test_env_key5.data()), // - tr_strdup(test_env_key6.data()), // + auto args = std::array{ + self_path_.c_str(), // + result_path.data(), // + arg_dump_env_.data(), // + test_env_key1.data(), // + test_env_key2.data(), // + test_env_key3.data(), // + test_env_key4.data(), // + test_env_key5.data(), // + test_env_key6.data(), // nullptr, // }; - auto env = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup_printf("%s=%s", test_env_key1.data(), test_env_value1.data()), - tr_strdup_printf("%s=%s", test_env_key2.data(), test_env_value2.data()), - tr_strdup_printf("%s=%s", test_env_key3.data(), test_env_value3.data()), - tr_strdup_printf("%s=%s", test_env_key5.data(), test_env_value5.data()), - nullptr, + auto const env = std::map{ + { test_env_key1, test_env_value1 }, + { test_env_key2, test_env_value2 }, + { test_env_key3, test_env_value3 }, + { test_env_key5, test_env_value5 }, }; setenv("FOO", "bar", true); // inherited setenv("ZOO", "tar", true); // overridden tr_error* error = nullptr; - bool const ret = tr_spawn_async(args.data(), env.data(), nullptr, &error); + bool const ret = tr_spawn_async(std::data(args), env, nullptr, &error); EXPECT_TRUE(ret); EXPECT_EQ(nullptr, error); @@ -248,11 +238,6 @@ TEST_P(SubprocessTest, SpawnAsyncEnv) EXPECT_FALSE(tr_sys_file_read_line(fd, buffer.data(), buffer.size(), nullptr)); tr_sys_file_close(fd, nullptr); - - for (auto& env_item : env) - { - tr_free(env_item); - } } TEST_P(SubprocessTest, SpawnAsyncCwdExplicit) @@ -260,16 +245,10 @@ TEST_P(SubprocessTest, SpawnAsyncCwdExplicit) auto const test_dir = sandbox_.path(); auto const result_path = buildSandboxPath("result.txt"); - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(self_path_.c_str()), - tr_strdup(result_path.data()), - tr_strdup(arg_dump_cwd_.data()), - nullptr - }; + auto const args = std::array{ self_path_.c_str(), result_path.data(), arg_dump_cwd_.data(), nullptr }; tr_error* error = nullptr; - bool const ret = tr_spawn_async(args.data(), nullptr, test_dir.c_str(), &error); + bool const ret = tr_spawn_async(std::data(args), {}, test_dir.c_str(), &error); EXPECT_TRUE(ret); EXPECT_EQ(nullptr, error); @@ -294,16 +273,10 @@ TEST_P(SubprocessTest, SpawnAsyncCwdInherit) auto const result_path = buildSandboxPath("result.txt"); auto const expected_cwd = nativeCwd(); - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(self_path_.c_str()), - tr_strdup(result_path.data()), - tr_strdup(arg_dump_cwd_.data()), - nullptr - }; + auto const args = std::array{ self_path_.c_str(), result_path.data(), arg_dump_cwd_.data(), nullptr }; tr_error* error = nullptr; - auto const ret = tr_spawn_async(args.data(), nullptr, nullptr, &error); + auto const ret = tr_spawn_async(std::data(args), {}, nullptr, &error); EXPECT_TRUE(ret); EXPECT_EQ(nullptr, error); @@ -323,16 +296,10 @@ TEST_P(SubprocessTest, SpawnAsyncCwdMissing) { auto const result_path = buildSandboxPath("result.txt"); - auto args = std::array{ - // FIXME(ckerr): remove tr_strdup()s after https://github.com/transmission/transmission/issues/1384 - tr_strdup(self_path_.c_str()), - tr_strdup(result_path.data()), - tr_strdup(arg_dump_cwd_.data()), - nullptr - }; + auto const args = std::array{ self_path_.c_str(), result_path.data(), arg_dump_cwd_.data(), nullptr }; tr_error* error = nullptr; - auto const ret = tr_spawn_async(args.data(), nullptr, TR_IF_WIN32("C:\\", "/") "tr-missing-test-work-dir", &error); + auto const ret = tr_spawn_async(std::data(args), {}, TR_IF_WIN32("C:\\", "/") "tr-missing-test-work-dir", &error); EXPECT_FALSE(ret); EXPECT_NE(nullptr, error); EXPECT_NE(0, error->code);