From 3366780926e3e610dfee397907dadae8cb82f0d1 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Mon, 31 Mar 2025 15:29:24 -0500 Subject: [PATCH 1/2] Handle spaces in start_process arguments on Windows The logic upon which this algorithm is based is derived from recommended practices on an MSDN blog. Signed-off-by: Scott K Logan --- CMakeLists.txt | 3 + src/process.c | 133 ++++++++++++++++++++++++++++++++++++++++-- test/test_process.cpp | 43 +++++++++++++- 3 files changed, 172 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 35913d3a..3cc9815a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -523,6 +523,9 @@ if(BUILD_TESTING) ) if(TARGET test_process) target_link_libraries(test_process ${PROJECT_NAME}) + target_compile_definitions(test_process PRIVATE + "CMAKE_COMMAND=${CMAKE_COMMAND}") + file(TOUCH "${CMAKE_CURRENT_BINARY_DIR}/file with space.txt") endif() ament_add_gtest(test_logging_custom_env test/test_logging_custom_env.cpp diff --git a/src/process.c b/src/process.c index 42e8f6ac..1ffcf71d 100644 --- a/src/process.c +++ b/src/process.c @@ -113,6 +113,115 @@ char * rcutils_get_executable_name(rcutils_allocator_t allocator) return executable_name; } +#if defined _WIN32 || defined __CYGWIN__ +static +rcutils_ret_t +append_backslashes(rcutils_char_array_t * char_array, size_t n) +{ + if (n <= 0) { + return RCUTILS_RET_OK; + } + + size_t current_strlen; + if (char_array->buffer_length == 0) { + current_strlen = 0; + } else { + // The buffer length always contains the trailing \0, so the strlen is one less than that. + current_strlen = char_array->buffer_length - 1; + } + + size_t new_length = current_strlen + n + 1; + rcutils_ret_t ret = rcutils_char_array_expand_as_needed(char_array, new_length); + if (RCUTILS_RET_OK != ret) { + return ret; + } + + memset(char_array->buffer + current_strlen, '\\', n); + char_array->buffer[new_length - 1] = '\0'; + + char_array->buffer_length = new_length; + + return RCUTILS_RET_OK; +} + +/** + * This algorithm is based on the MSDN blog "everyone quotes command line arguments the wrong way". + * See https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way + */ +static +rcutils_ret_t +build_command_line( + const rcutils_string_array_t * string_array, + rcutils_char_array_t * char_array) +{ + rcutils_ret_t ret; + + for (size_t i = 0; i < string_array->size; i++) { + // Append argument separator + if (i != 0) { + ret = rcutils_char_array_strncat(char_array, " ", 1); + if (RCUTILS_RET_OK != ret) { + return ret; + } + } + + // Append argument which doesn't need to be quoted + size_t arg_length = strlen(string_array->data[i]); + if (strcspn(string_array->data[i], " \t\n\v\"") >= arg_length) { + ret = rcutils_char_array_strncat(char_array, string_array->data[i], arg_length); + if (RCUTILS_RET_OK != ret) { + return ret; + } + continue; + } + + // Append opening quote + ret = rcutils_char_array_strncat(char_array, "\"", 1); + if (RCUTILS_RET_OK != ret) { + return ret; + } + + for (const char * c = string_array->data[i]; ; c++) { + size_t backslash_count = 0; + while ('\\' == *c) { + backslash_count++; + c++; + } + + if ('\0' == *c) { + ret = append_backslashes(char_array, backslash_count * 2); + if (RCUTILS_RET_OK != ret) { + return ret; + } + break; + } + + if ('"' == *c) { + backslash_count = backslash_count * 2 + 1; + } + + ret = append_backslashes(char_array, backslash_count); + if (RCUTILS_RET_OK != ret) { + return ret; + } + + ret = rcutils_char_array_strncat(char_array, c, 1); + if (RCUTILS_RET_OK != ret) { + return ret; + } + } + + // Append closing quote + ret = rcutils_char_array_strncat(char_array, "\"", 1); + if (RCUTILS_RET_OK != ret) { + return ret; + } + } + + return RCUTILS_RET_OK; +} +#endif + rcutils_process_t * rcutils_start_process( const rcutils_string_array_t * args, @@ -135,8 +244,18 @@ rcutils_start_process( process->allocator = *allocator; #if defined _WIN32 || defined __CYGWIN__ - LPSTR cmd = rcutils_join(args, " ", *allocator); - if (NULL == cmd) { + rcutils_char_array_t cmd = rcutils_get_zero_initialized_char_array(); + rcutils_ret_t ret = rcutils_char_array_init(&cmd, 0, allocator); + if (RCUTILS_RET_OK != ret) { + rcutils_process_close(process); + return NULL; + } + + ret = build_command_line(args, &cmd); + if (RCUTILS_RET_OK != ret) { + if (RCUTILS_RET_OK != rcutils_char_array_fini(&cmd)) { + RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to fini array.\n"); + } rcutils_process_close(process); return NULL; } @@ -144,16 +263,20 @@ rcutils_start_process( STARTUPINFO start_info = {sizeof(start_info)}; PROCESS_INFORMATION process_info = {0}; if (!CreateProcess( - NULL, cmd, NULL, NULL, TRUE, 0, + NULL, cmd.buffer, NULL, NULL, TRUE, 0, NULL, NULL, &start_info, &process_info)) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING("Failed to create process: %lu", GetLastError()); - allocator->deallocate(cmd, &allocator->state); + if (RCUTILS_RET_OK != rcutils_char_array_fini(&cmd)) { + RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to fini array.\n"); + } rcutils_process_close(process); return NULL; } - allocator->deallocate(cmd, &allocator->state); + if (RCUTILS_RET_OK != rcutils_char_array_fini(&cmd)) { + RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to fini array.\n"); + } CloseHandle(process_info.hThread); diff --git a/test/test_process.cpp b/test/test_process.cpp index 909bdff2..8dccde3b 100644 --- a/test/test_process.cpp +++ b/test/test_process.cpp @@ -20,6 +20,8 @@ #include "rcutils/error_handling.h" #include "rcutils/process.h" +static const char * const g_cmake_command = RCUTILS_STRINGIFY(CMAKE_COMMAND); + TEST(TestProcess, test_get_pid) { EXPECT_NE(rcutils_get_pid(), 0); } @@ -52,9 +54,12 @@ TEST(TestProcess, test_process_creation) { rcutils_ret_t ret = RCUTILS_RET_OK; int exit_code = 42; - ret = rcutils_string_array_init(&args, 1, &allocator); + ret = rcutils_string_array_init(&args, 4, &allocator); ASSERT_EQ(RCUTILS_RET_OK, ret); - args.data[0] = strdup("whoami"); + args.data[0] = strdup(g_cmake_command); + args.data[1] = strdup("-E"); + args.data[2] = strdup("echo"); + args.data[3] = strdup(""); EXPECT_EQ(nullptr, rcutils_start_process(NULL, &allocator)); rcutils_reset_error(); @@ -78,6 +83,40 @@ TEST(TestProcess, test_process_creation) { rcutils_process_close(process); + // cmake -E cat "file with space.txt" (returns 0) + ret = rcutils_string_array_resize(&args, 4); + ASSERT_EQ(RCUTILS_RET_OK, ret); + args.data[0] = strdup(g_cmake_command); + args.data[1] = strdup("-E"); + args.data[2] = strdup("cat"); + args.data[3] = strdup("file with space.txt"); + + process = rcutils_start_process(&args, &allocator); + EXPECT_NE(nullptr, process); + + ret = rcutils_process_wait(process, &exit_code); + EXPECT_EQ(RCUTILS_RET_OK, ret); + EXPECT_EQ(0, exit_code); + + rcutils_process_close(process); + + // cmake -E false (returns 1) + ret = rcutils_string_array_resize(&args, 3); + ASSERT_EQ(RCUTILS_RET_OK, ret); + args.data[0] = strdup(g_cmake_command); + args.data[1] = strdup("-E"); + allocator.deallocate(args.data[2], &allocator.state); + args.data[2] = strdup("false"); + + process = rcutils_start_process(&args, &allocator); + EXPECT_NE(nullptr, process); + + ret = rcutils_process_wait(process, &exit_code); + EXPECT_EQ(RCUTILS_RET_OK, ret); + EXPECT_EQ(1, exit_code); + + rcutils_process_close(process); + ret = rcutils_string_array_fini(&args); ASSERT_EQ(RCUTILS_RET_OK, ret); } From 2238b122428675b7c7c3ccaaabf519853399eac2 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Tue, 1 Apr 2025 01:16:57 -0500 Subject: [PATCH 2/2] PR feedback: Rename 'n' to 'num_backslashes' Signed-off-by: Scott K Logan --- src/process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/process.c b/src/process.c index 1ffcf71d..d62c3295 100644 --- a/src/process.c +++ b/src/process.c @@ -116,9 +116,9 @@ char * rcutils_get_executable_name(rcutils_allocator_t allocator) #if defined _WIN32 || defined __CYGWIN__ static rcutils_ret_t -append_backslashes(rcutils_char_array_t * char_array, size_t n) +append_backslashes(rcutils_char_array_t * char_array, size_t num_backslashes) { - if (n <= 0) { + if (num_backslashes <= 0) { return RCUTILS_RET_OK; } @@ -130,13 +130,13 @@ append_backslashes(rcutils_char_array_t * char_array, size_t n) current_strlen = char_array->buffer_length - 1; } - size_t new_length = current_strlen + n + 1; + size_t new_length = current_strlen + num_backslashes + 1; rcutils_ret_t ret = rcutils_char_array_expand_as_needed(char_array, new_length); if (RCUTILS_RET_OK != ret) { return ret; } - memset(char_array->buffer + current_strlen, '\\', n); + memset(char_array->buffer + current_strlen, '\\', num_backslashes); char_array->buffer[new_length - 1] = '\0'; char_array->buffer_length = new_length;