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>

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;

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

rcl/test/rcl/test_arguments.cpp

@@ -935,3 +935,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;
+}