Fix bug error handling get_param_files (#743)

* Add bomb allocator * Fix memory checked for error * Add tests for bad alloc * Fix break statement * Add static keyword to methods added Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
ros2 · Aug 13, 2020 · ed449a5 · ed449a5
1 parent f90c01c
commit ed449a5
Show file tree

Hide file tree

Showing 3 changed files with 111 additions and 4 deletions.
diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c
@@ -93,10 +93,7 @@ rcl_arguments_get_param_files(
     if (NULL == (*parameter_files)[i]) {
       // deallocate allocated memory
       for (int r = i; r >= 0; --r) {
-        if (NULL == (*parameter_files[r])) {
-          break;
-        }
-        allocator.deallocate((*parameter_files[r]), allocator.state);
+        allocator.deallocate((*parameter_files)[r], allocator.state);
       }
       allocator.deallocate((*parameter_files), allocator.state);
       (*parameter_files) = NULL;

diff --git a/rcl/test/rcl/allocator_testing_utils.h b/rcl/test/rcl/allocator_testing_utils.h
@@ -87,6 +87,80 @@ set_failing_allocator_is_failing(rcutils_allocator_t & failing_allocator, bool s
   ((__failing_allocator_state *)failing_allocator.state)->is_failing = state;
 }
 
+typedef struct time_bomb_allocator_state
+{
+  int count_until_failure;
+} time_bomb_allocator_state;
+
+static void * time_bomb_malloc(size_t size, void * state)
+{
+  time_bomb_allocator_state * time_bomb_state = (time_bomb_allocator_state *)state;
+  if (time_bomb_state->count_until_failure >= 0 &&
+    time_bomb_state->count_until_failure-- == 0)
+  {
+    return nullptr;
+  }
+  return rcutils_get_default_allocator().allocate(size, rcutils_get_default_allocator().state);
+}
+
+static void *
+time_bomb_realloc(void * pointer, size_t size, void * state)
+{
+  time_bomb_allocator_state * time_bomb_state = (time_bomb_allocator_state *)state;
+  if (time_bomb_state->count_until_failure >= 0 &&
+    time_bomb_state->count_until_failure-- == 0)
+  {
+    return nullptr;
+  }
+  return rcutils_get_default_allocator().reallocate(
+    pointer, size, rcutils_get_default_allocator().state);
+}
+
+static void
+time_bomb_free(void * pointer, void * state)
+{
+  time_bomb_allocator_state * time_bomb_state = (time_bomb_allocator_state *)state;
+  if (time_bomb_state->count_until_failure >= 0 &&
+    time_bomb_state->count_until_failure-- == 0)
+  {
+    return;
+  }
+  rcutils_get_default_allocator().deallocate(pointer, rcutils_get_default_allocator().state);
+}
+
+static void *
+time_bomb_calloc(size_t number_of_elements, size_t size_of_element, void * state)
+{
+  time_bomb_allocator_state * time_bomb_state = (time_bomb_allocator_state *)state;
+  if (time_bomb_state->count_until_failure >= 0 &&
+    time_bomb_state->count_until_failure-- == 0)
+  {
+    return nullptr;
+  }
+  return rcutils_get_default_allocator().zero_allocate(
+    number_of_elements, size_of_element, rcutils_get_default_allocator().state);
+}
+
+static inline rcutils_allocator_t
+get_time_bombed_allocator(void)
+{
+  static time_bomb_allocator_state state;
+  state.count_until_failure = 1;
+  auto time_bombed_allocator = rcutils_get_default_allocator();
+  time_bombed_allocator.allocate = time_bomb_malloc;
+  time_bombed_allocator.deallocate = time_bomb_free;
+  time_bombed_allocator.reallocate = time_bomb_realloc;
+  time_bombed_allocator.zero_allocate = time_bomb_calloc;
+  time_bombed_allocator.state = &state;
+  return time_bombed_allocator;
+}
+
+static inline void
+set_time_bombed_allocator_count(rcutils_allocator_t & time_bombed_allocator, int count)
+{
+  ((time_bomb_allocator_state *)time_bombed_allocator.state)->count_until_failure = count;
+}
+
 #ifdef __cplusplus
 }
 #endif

diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp
@@ -1029,3 +1029,39 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_overrides
   ASSERT_TRUE(NULL != param_value->string_value);
   EXPECT_STREQ("foo", param_value->string_value);
 }
+
+TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_alloc_get_param_files) {
+  const std::string parameters_filepath1 = (test_path / "test_parameters.1.yaml").string();
+  const std::string parameters_filepath2 = (test_path / "test_parameters.2.yaml").string();
+  const char * const argv[] = {
+    "process_name", "--ros-args", "--params-file", parameters_filepath1.c_str(),
+    "-r", "__ns:=/namespace", "random:=arg", "--params-file", parameters_filepath2.c_str()
+  };
+  const int argc = sizeof(argv) / sizeof(const char *);
+
+  rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
+  {
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
+
+  int parameter_filecount = rcl_arguments_get_param_files_count(&parsed_args);
+  EXPECT_EQ(2, parameter_filecount);
+
+  // Configure allocator to fail at different points of the code
+  rcl_allocator_t bomb_alloc = get_time_bombed_allocator();
+  set_time_bombed_allocator_count(bomb_alloc, 0);
+  char ** parameter_files = NULL;
+  ret = rcl_arguments_get_param_files(&parsed_args, bomb_alloc, &parameter_files);
+  EXPECT_EQ(RCL_RET_BAD_ALLOC, ret) << rcl_get_error_string().str;
+
+  set_time_bombed_allocator_count(bomb_alloc, 1);
+  ret = rcl_arguments_get_param_files(&parsed_args, bomb_alloc, &parameter_files);
+  EXPECT_EQ(RCL_RET_BAD_ALLOC, ret) << rcl_get_error_string().str;
+
+  set_time_bombed_allocator_count(bomb_alloc, 2);
+  ret = rcl_arguments_get_param_files(&parsed_args, bomb_alloc, &parameter_files);
+  EXPECT_EQ(RCL_RET_BAD_ALLOC, ret) << rcl_get_error_string().str;
+}