Increase rcl_lifecycle test coverage and add more safety checks (#649)

* Increase test coverage and add more safety checks Signed-off-by: Stephen Brawner <brawner@gmail.com> * More coverage Signed-off-by: Stephen Brawner <brawner@gmail.com> * Concatenating error messages Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com> * PR Fixup Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com> * Forgot a Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com> * PR Fixup Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com> * Moving var to top Signed-off-by: Stephen Brawner <stephenbrawner@verbsurgical.com> Co-authored-by: Stephen Brawner <stephenbrawner@verbsurgical.com>
ros2 · May 22, 2020 · 7146919 · 7146919
1 parent 30e0536
commit 7146919
Show file tree

Hide file tree

Showing 7 changed files with 591 additions and 19 deletions.
diff --git a/rcl_lifecycle/CMakeLists.txt b/rcl_lifecycle/CMakeLists.txt
@@ -89,6 +89,16 @@ if(BUILD_TESTING)
     )
     target_link_libraries(test_multiple_instances ${PROJECT_NAME})
   endif()
+  ament_add_gtest(test_rcl_lifecycle
+    test/test_rcl_lifecycle.cpp
+  )
+  if(TARGET test_rcl_lifecycle)
+    ament_target_dependencies(test_rcl_lifecycle
+      "rcl"
+      "osrf_testing_tools_cpp"
+    )
+    target_link_libraries(test_rcl_lifecycle ${PROJECT_NAME})
+  endif()
   ament_add_gtest(test_transition_map
     test/test_transition_map.cpp
   )

diff --git a/rcl_lifecycle/src/default_state_machine.c b/rcl_lifecycle/src/default_state_machine.c
@@ -650,13 +650,15 @@ _register_transitions(
   return ret;
 }
 
-// default implementation as despicted on
+// default implementation as depicted on
 // design.ros2.org
 rcl_ret_t
 rcl_lifecycle_init_default_state_machine(
   rcl_lifecycle_state_machine_t * state_machine, const rcutils_allocator_t * allocator)
 {
   rcl_ret_t ret = RCL_RET_ERROR;
+  // Used for concatenating error messages in the fail: block.
+  const char * fail_error_message = "";
 
   // ***************************
   // register all primary states
@@ -691,8 +693,26 @@ rcl_lifecycle_init_default_state_machine(
   return ret;
 
 fail:
+  // If rcl_lifecycle_transition_map_fini() fails, it will clobber the error string here.
+  // Concatenate the error strings if that happens
+  if (rcl_error_is_set()) {
+    fail_error_message = rcl_get_error_string().str;
+  }
+
   if (rcl_lifecycle_transition_map_fini(&state_machine->transition_map, allocator) != RCL_RET_OK) {
-    RCL_SET_ERROR_MSG("could not free lifecycle transition map. Leaking memory!\n");
+    const char * fini_error = "";
+    if (rcl_error_is_set()) {
+      fini_error = rcl_get_error_string().str;
+      rcl_reset_error();
+    }
+    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
+      "Freeing transition map failed while handling a previous error. Leaking memory!"
+      "\nOriginal error:\n\t%s\nError encountered in rcl_lifecycle_transition_map_fini():\n\t%s\n",
+      fail_error_message, fini_error);
+  }
+
+  if (!rcl_error_is_set()) {
+    RCL_SET_ERROR_MSG("Unspecified error in default_state_machine _register_transitions()");
   }
   return RCL_RET_ERROR;
 }

diff --git a/rcl_lifecycle/src/rcl_lifecycle.c b/rcl_lifecycle/src/rcl_lifecycle.c
@@ -40,6 +40,8 @@ rcl_lifecycle_get_zero_initialized_state()
   rcl_lifecycle_state_t state;
   state.id = 0;
   state.label = NULL;
+  state.valid_transitions = NULL;
+  state.valid_transition_size = 0;
   return state;
 }
 
@@ -58,6 +60,10 @@ rcl_lifecycle_state_init(
     RCL_SET_ERROR_MSG("state pointer is null\n");
     return RCL_RET_ERROR;
   }
+  if (!label) {
+    RCL_SET_ERROR_MSG("State label is null\n");
+    return RCL_RET_ERROR;
+  }
 
   state->id = id;
   state->label = rcutils_strndup(label, strlen(label), *allocator);
@@ -118,7 +124,22 @@ rcl_lifecycle_transition_init(
 
   if (!transition) {
     RCL_SET_ERROR_MSG("transition pointer is null\n");
-    return RCL_RET_OK;
+    return RCL_RET_ERROR;
+  }
+
+  if (!label) {
+    RCL_SET_ERROR_MSG("label pointer is null\n");
+    return RCL_RET_ERROR;
+  }
+
+  if (!start) {
+    RCL_SET_ERROR_MSG("start state pointer is null\n");
+    return RCL_RET_ERROR;
+  }
+
+  if (!goal) {
+    RCL_SET_ERROR_MSG("goal state pointer is null\n");
+    return RCL_RET_ERROR;
   }
 
   transition->start = start;
@@ -140,7 +161,7 @@ rcl_lifecycle_transition_fini(
   const rcl_allocator_t * allocator)
 {
   if (!allocator) {
-    RCL_SET_ERROR_MSG("can't initialize transition, no allocator given\n");
+    RCL_SET_ERROR_MSG("can't finalize transition, no allocator given\n");
     return RCL_RET_ERROR;
   }
   // it is already NULL
@@ -192,6 +213,14 @@ rcl_lifecycle_state_machine_init(
   bool default_states,
   const rcl_allocator_t * allocator)
 {
+  if (!state_machine) {
+    RCL_SET_ERROR_MSG("State machine is null\n");
+    return RCL_RET_ERROR;
+  }
+  if (!node_handle) {
+    RCL_SET_ERROR_MSG("Node handle is null\n");
+    return RCL_RET_ERROR;
+  }
   if (!allocator) {
     RCL_SET_ERROR_MSG("can't initialize state machine, no allocator given\n");
     return RCL_RET_ERROR;
@@ -207,15 +236,12 @@ rcl_lifecycle_state_machine_init(
   }
 
   if (default_states) {
-    rcl_ret_t ret =
-      rcl_lifecycle_init_default_state_machine(state_machine, allocator);
+    ret = rcl_lifecycle_init_default_state_machine(state_machine, allocator);
     if (ret != RCL_RET_OK) {
       // init default state machine might have allocated memory,
       // so we have to call fini
-      if (rcl_lifecycle_state_machine_fini(state_machine, node_handle, allocator) != RCL_RET_OK) {
-        // error already set
-        return RCL_RET_ERROR;
-      }
+      ret = rcl_lifecycle_state_machine_fini(state_machine, node_handle, allocator);
+      return RCL_RET_ERROR;
     }
   }
 

diff --git a/rcl_lifecycle/src/transition_map.c b/rcl_lifecycle/src/transition_map.c
@@ -53,6 +53,11 @@ rcl_lifecycle_transition_map_fini(
   rcl_lifecycle_transition_map_t * transition_map,
   const rcutils_allocator_t * allocator)
 {
+  if (!allocator) {
+    RCL_SET_ERROR_MSG("can't free transition map, no allocator given\n");
+    return RCL_RET_ERROR;
+  }
+
   rcl_ret_t fcn_ret = RCL_RET_OK;
 
   // free valid transitions for all states
@@ -87,15 +92,16 @@ rcl_lifecycle_register_state(
     allocator, "invalid allocator", return RCUTILS_RET_INVALID_ARGUMENT)
 
   // add new primary state memory
-  transition_map->states_size += 1;
+  unsigned int new_states_size = transition_map->states_size + 1;
   rcl_lifecycle_state_t * new_states = allocator->reallocate(
     transition_map->states,
-    transition_map->states_size * sizeof(rcl_lifecycle_state_t),
+    new_states_size * sizeof(rcl_lifecycle_state_t),
     allocator->state);
   if (!new_states) {
     RCL_SET_ERROR_MSG("failed to reallocate memory for new states");
     return RCL_RET_ERROR;
   }
+  transition_map->states_size = new_states_size;
   transition_map->states = new_states;
   transition_map->states[transition_map->states_size - 1] = state;
 
@@ -117,32 +123,39 @@ rcl_lifecycle_register_transition(
     return RCL_RET_ERROR;
   }
 
-  // we add a new transition, so increase the size
-  transition_map->transitions_size += 1;
+  rcl_lifecycle_state_t * goal = rcl_lifecycle_get_state(transition_map, transition.goal->id);
+  if (!goal) {
+    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING("state %u is not registered\n", transition.goal->id);
+    return RCL_RET_ERROR;
+  }
+  // Attempt to add new transition, don't update map if it fails
+  unsigned int new_transitions_size = transition_map->transitions_size + 1;
   rcl_lifecycle_transition_t * new_transitions = allocator->reallocate(
     transition_map->transitions,
-    transition_map->transitions_size * sizeof(rcl_lifecycle_transition_t),
+    new_transitions_size * sizeof(rcl_lifecycle_transition_t),
     allocator->state);
   if (!new_transitions) {
     RCL_SET_ERROR_MSG("failed to reallocate memory for new transitions");
     return RCL_RET_BAD_ALLOC;
   }
+  transition_map->transitions_size = new_transitions_size;
   transition_map->transitions = new_transitions;
   // finally set the new transition to the end of the array
   transition_map->transitions[transition_map->transitions_size - 1] = transition;
 
   // we have to copy the transitons here once more to the actual state
   // as we can't assign only the pointer. This pointer gets invalidated whenever
   // we add a new transition and re-shuffle/re-allocate new memory for it.
-  state->valid_transition_size += 1;
+  unsigned int new_valid_transitions_size = state->valid_transition_size + 1;
   rcl_lifecycle_transition_t * new_valid_transitions = allocator->reallocate(
     state->valid_transitions,
-    state->valid_transition_size * sizeof(rcl_lifecycle_transition_t),
+    new_valid_transitions_size * sizeof(rcl_lifecycle_transition_t),
     allocator->state);
   if (!new_valid_transitions) {
     RCL_SET_ERROR_MSG("failed to reallocate memory for new transitions on state");
     return RCL_RET_ERROR;
   }
+  state->valid_transition_size = new_valid_transitions_size;
   state->valid_transitions = new_valid_transitions;
 
   state->valid_transitions[state->valid_transition_size - 1] = transition;

diff --git a/rcl_lifecycle/test/test_default_state_machine.cpp b/rcl_lifecycle/test/test_default_state_machine.cpp
@@ -113,7 +113,13 @@ TEST_F(TestDefaultStateMachine, zero_init) {
 TEST_F(TestDefaultStateMachine, default_init) {
   rcl_lifecycle_state_machine_t state_machine = rcl_lifecycle_get_zero_initialized_state_machine();
 
-  auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
+  // Because this init method is so complex, the succession of failures caused by a null
+  // allocator will result in several error messages overwriting themselves.
+  auto ret = rcl_lifecycle_init_default_state_machine(&state_machine, nullptr);
+  EXPECT_EQ(RCL_RET_ERROR, ret);
+  rcutils_reset_error();
+
+  ret = rcl_lifecycle_init_default_state_machine(&state_machine, this->allocator);
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 
   ret = rcl_lifecycle_state_machine_fini(&state_machine, this->node_ptr, this->allocator);