Handle spaces in start_process arguments on Windows (ros2#494)

Signed-off-by: Scott K Logan <logans@cottsay.net>

Author: cottsay

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

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 num_backslashes)
+{
+  if (num_backslashes <= 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 + 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, '\\', num_backslashes);
+  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,25 +244,39 @@ 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;
   }
 
   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);
 

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);
 }