fix memory leaks in test_security_directory (#420)

* fix memory leak in test_security_directory

Signed-off-by: Abby Xu <abbyxu@amazon.com>

* fix linter errors

Signed-off-by: Abby Xu <abbyxu@amazon.com>

rcl/src/rcl/security_directory.c

@@ -219,13 +219,15 @@ char * rcl_get_secure_root(
   char * lookup_strategy = NULL;
   char * node_secure_root = NULL;
   if (ros_secure_node_override) {
-    node_secure_root = ros_secure_root_env;
+    node_secure_root = (char *)allocator->allocate(ros_secure_root_size + 1, allocator->state);
+    memcpy(node_secure_root, ros_secure_root_env, ros_secure_root_size + 1);
     lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_NODE_OVERRIDE];
 
   } else {
     // Check which lookup method to use and invoke the relevant function.
     const char * ros_security_lookup_type = NULL;
     if (rcutils_get_env(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME, &ros_security_lookup_type)) {
+      allocator->deallocate(ros_secure_root_env, allocator->state);
       return NULL;
     }
     if (0 == strcmp(ros_security_lookup_type,
@@ -240,19 +242,23 @@ char * rcl_get_secure_root(
       lookup_strategy = g_security_lookup_type_strings[ROS_SECURITY_LOOKUP_MATCH_EXACT];
     }
   }
-  // Check node_secure_root is not NULL before checking directory
+
-  if (NULL == node_secure_root) {
+  if (NULL == node_secure_root || !rcutils_is_directory(node_secure_root)) {
-    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
+    // Check node_secure_root is not NULL before checking directory
-      "SECURITY ERROR: unable to find a folder matching the node name in %s%s."
+    if (NULL == node_secure_root) {
-      "Lookup strategy: %s",
+      RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
-      ros_secure_root_env, node_namespace, lookup_strategy);
+        "SECURITY ERROR: unable to find a folder matching the node name in %s%s."
-  } else if (!rcutils_is_directory(node_secure_root)) {
+        "Lookup strategy: %s",
-    RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
+        ros_secure_root_env, node_namespace, lookup_strategy);
-      "SECURITY ERROR: directory %s does not exist. Lookup strategy: %s",
+    } else {
-      node_secure_root, lookup_strategy);
+      RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
+        "SECURITY ERROR: directory %s does not exist. Lookup strategy: %s",
+        node_secure_root, lookup_strategy);
+    }
+    allocator->deallocate(ros_secure_root_env, allocator->state);
     allocator->deallocate(node_secure_root, allocator->state);
-  } else {
+    return NULL;
-    return node_secure_root;
   }
-  return NULL;
+  allocator->deallocate(ros_secure_root_env, allocator->state);
+  return node_secure_root;
 }

rcl/test/CMakeLists.txt

@@ -313,6 +313,7 @@ rcl_add_custom_gtest(test_security_directory
   SRCS rcl/test_security_directory.cpp
   APPEND_LIBRARY_DIRS ${extra_lib_dirs}
   LIBRARIES ${PROJECT_NAME}
+  AMENT_DEPENDENCIES "osrf_testing_tools_cpp"
 )
 
 # Install test resources

rcl/test/rcl/test_security_directory.cpp

@@ -18,6 +18,8 @@
 #include <algorithm>
 #include "rcl/security_directory.h"
 #include "rcutils/filesystem.h"
+#include "osrf_testing_tools_cpp/scope_exit.hpp"
+#include "rcl/error_handling.h"
 
 #define ROOT_NAMESPACE "/"
 #define TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME "test_security_directory"
@@ -50,17 +52,53 @@ static int unsetenv_wrapper(const char * var_name)
 class TestGetSecureRoot : public ::testing::Test
 {
 protected:
-  void SetUp() override
+  void SetUp() final
   {
+    // Reset rcutil error global state in case a previously
+    // running test has failed.
+    rcl_reset_error();
+
     // Always make sure the variable we set is unset at the beginning of a test
     unsetenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME);
     unsetenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME);
     unsetenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME);
+    allocator = rcl_get_default_allocator();
+    root_path = nullptr;
+    secure_root = nullptr;
+    base_lookup_dir_fqn = nullptr;
   }
+
+  void TearDown() final
+  {
+    OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+      allocator.deallocate(root_path, allocator.state);
+    });
+    OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+      allocator.deallocate(secure_root, allocator.state);
+    });
+    OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+      allocator.deallocate(base_lookup_dir_fqn, allocator.state);
+    });
+  }
+
+  void set_base_lookup_dir_fqn(const char * resource_dir, const char * resource_dir_name)
+  {
+    base_lookup_dir_fqn = rcutils_join_path(resource_dir,
+        resource_dir_name, allocator);
+    std::string putenv_input = ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=";
+    putenv_input += base_lookup_dir_fqn;
+    memcpy(g_envstring, putenv_input.c_str(),
+      std::min(putenv_input.length(), sizeof(g_envstring) - 1));
+    putenv_wrapper(g_envstring);
+  }
+
+  rcl_allocator_t allocator;
+  char * root_path;
+  char * secure_root;
+  char * base_lookup_dir_fqn;
 };
 
 TEST_F(TestGetSecureRoot, failureScenarios) {
-  rcl_allocator_t allocator = rcl_get_default_allocator();
   ASSERT_EQ(rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator),
     (char *) NULL);
 
@@ -75,9 +113,9 @@ TEST_F(TestGetSecureRoot, failureScenarios) {
     (char *) NULL);
 }
 
-TEST_F(TestGetSecureRoot, successScenarios) {
+TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) {
-  rcl_allocator_t allocator = rcl_get_default_allocator();
   putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+
   /* --------------------------
    * Namespace  : Custom (local)
    * Match type : Exact
@@ -86,9 +124,15 @@ TEST_F(TestGetSecureRoot, successScenarios) {
    * Namespace: /test_security_directory
    * Node: dummy_node
    */
-  std::string secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
+  secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
+  std::string secure_root_str(secure_root);
   ASSERT_STREQ(TEST_NODE_NAME,
-    secure_root.substr(secure_root.size() - (sizeof(TEST_NODE_NAME) - 1)).c_str());
+    secure_root_str.substr(secure_root_str.size() - (sizeof(TEST_NODE_NAME) - 1)).c_str());
+}
+
+TEST_F(TestGetSecureRoot, successScenarios_local_prefixMatch) {
+  putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+  secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
 
   /* --------------------------
    * Namespace  : Custom (local)
@@ -97,22 +141,22 @@ TEST_F(TestGetSecureRoot, successScenarios) {
    * Root: ${CMAKE_BINARY_DIR}/tests/resources
    * Namespace: /test_security_directory
    * Node: dummy_node_and_some_suffix_added */
-  ASSERT_STRNE(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE,
+  root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added",
-    &allocator),
+      TEST_NODE_NAMESPACE, &allocator);
-    secure_root.c_str());
+  ASSERT_STRNE(root_path, secure_root);
   putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
-  ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", TEST_NODE_NAMESPACE,
+  root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added",
-    &allocator),
+      TEST_NODE_NAMESPACE, &allocator);
-    secure_root.c_str());
+  ASSERT_STREQ(root_path, secure_root);
+}
+
+TEST_F(TestGetSecureRoot, successScenarios_root_exactMatch) {
+  putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+  putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
+  secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
 
   /* Include the namespace as part of the root security directory and test root namespace */
-  char * base_lookup_dir_fqn = rcutils_join_path(TEST_RESOURCES_DIRECTORY,
+  set_base_lookup_dir_fqn(TEST_RESOURCES_DIRECTORY, TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
-      TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME, allocator);
-  std::string putenv_input = ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=";
-  putenv_input += base_lookup_dir_fqn;
-  memcpy(g_envstring, putenv_input.c_str(),
-    std::min(putenv_input.length(), sizeof(g_envstring) - 1));
-  putenv_wrapper(g_envstring);
   /* --------------------------
    * Namespace  : Root
    * Match type : Exact
@@ -120,14 +164,17 @@ TEST_F(TestGetSecureRoot, successScenarios) {
    * Root: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory
    * Namespace: /
    * Node: dummy_node */
-  ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE,
+  root_path = rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE, &allocator);
-    &allocator),
+  ASSERT_STREQ(root_path, secure_root);
-    secure_root.c_str());
+}
-  putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_EXACT");
-  ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE,
-    &allocator),
-    secure_root.c_str());
 
+TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) {
+  putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+  putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
+  secure_root = rcl_get_secure_root(TEST_NODE_NAME, TEST_NODE_NAMESPACE, &allocator);
+
+  /* Include the namespace as part of the root security directory and test root namespace */
+  set_base_lookup_dir_fqn(TEST_RESOURCES_DIRECTORY, TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME);
   /* --------------------------
    * Namespace  : Root
    * Match type : Prefix
@@ -135,27 +182,30 @@ TEST_F(TestGetSecureRoot, successScenarios) {
    * Root dir: ${CMAKE_BINARY_DIR}/tests/resources/test_security_directory
    * Namespace: /
    * Node: dummy_node_and_some_suffix_added */
-  ASSERT_STRNE(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE,
+  root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added",
-    &allocator),
+      ROOT_NAMESPACE, &allocator);
-    secure_root.c_str());
+  ASSERT_STREQ(root_path, secure_root);
-  putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX");
-  ASSERT_STREQ(rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", ROOT_NAMESPACE,
-    &allocator),
-    secure_root.c_str());
 }
 
-TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride) {
+TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) {
-  rcl_allocator_t allocator = rcl_get_default_allocator();
   /* Specify a valid directory */
   putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
-  ASSERT_STREQ(rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter",
+  root_path = rcl_get_secure_root("name shouldn't matter",
-    &allocator), TEST_RESOURCES_DIRECTORY);
+      "namespace shouldn't matter", &allocator);
+  ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
+}
 
+TEST_F(TestGetSecureRoot,
+  nodeSecurityDirectoryOverride_validDirectory_overrideRootDirectoryAttempt) {
   /* Setting root dir has no effect */
+  putenv_wrapper(ROS_SECURITY_NODE_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
+  root_path = rcl_get_secure_root("name shouldn't matter",
+      "namespace shouldn't matter", &allocator);
   putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY);
-  ASSERT_STREQ(rcl_get_secure_root("name shouldn't matter", "namespace shouldn't matter",
+  ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY);
-    &allocator), TEST_RESOURCES_DIRECTORY);
+}
 
+TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_invalidDirectory) {
   /* The override provided should exist. Providing correct node/namespace/root dir won't help
    * if the node override is invalid. */
   putenv_wrapper(