From 0b06f0f24fd662f19e79a1e17d56cff4425fcda4 Mon Sep 17 00:00:00 2001 From: Abby Xu Date: Wed, 8 May 2019 15:10:25 -0700 Subject: [PATCH 1/2] fix memory leak in test_security_directory Signed-off-by: Abby Xu --- rcl/src/rcl/security_directory.c | 36 ++++--- rcl/test/CMakeLists.txt | 1 + rcl/test/rcl/test_security_directory.cpp | 128 ++++++++++++++++------- 3 files changed, 111 insertions(+), 54 deletions(-) diff --git a/rcl/src/rcl/security_directory.c b/rcl/src/rcl/security_directory.c index 6559eb824..ab353f862 100644 --- a/rcl/src/rcl/security_directory.c +++ b/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) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "SECURITY ERROR: unable to find a folder matching the node name in %s%s." - "Lookup strategy: %s", - ros_secure_root_env, node_namespace, lookup_strategy); - } else if (!rcutils_is_directory(node_secure_root)) { - RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( - "SECURITY ERROR: directory %s does not exist. Lookup strategy: %s", - node_secure_root, lookup_strategy); + + if (NULL == node_secure_root || !rcutils_is_directory(node_secure_root)) { + // Check node_secure_root is not NULL before checking directory + if (NULL == node_secure_root) { + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "SECURITY ERROR: unable to find a folder matching the node name in %s%s." + "Lookup strategy: %s", + ros_secure_root_env, node_namespace, lookup_strategy); + } else { + 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 node_secure_root; + return NULL; } - return NULL; -} + allocator->deallocate(ros_secure_root_env, allocator->state); + return node_secure_root; +} \ No newline at end of file diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index 5ffae7cba..ecf4126f7 100644 --- a/rcl/test/CMakeLists.txt +++ b/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 diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security_directory.cpp index 9ac2f9259..806f82f82 100644 --- a/rcl/test/rcl/test_security_directory.cpp +++ b/rcl/test/rcl/test_security_directory.cpp @@ -18,6 +18,8 @@ #include #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) { - rcl_allocator_t allocator = rcl_get_default_allocator(); +TEST_F(TestGetSecureRoot, successScenarios_local_exactMatch) { 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, - &allocator), - secure_root.c_str()); + root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", + TEST_NODE_NAMESPACE, &allocator); + 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, - &allocator), - secure_root.c_str()); + root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", + TEST_NODE_NAMESPACE, &allocator); + 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, - 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); + set_base_lookup_dir_fqn(TEST_RESOURCES_DIRECTORY, TEST_SECURITY_DIRECTORY_RESOURCES_DIR_NAME); /* -------------------------- * 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, - &allocator), - 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()); + root_path = rcl_get_secure_root(TEST_NODE_NAME, ROOT_NAMESPACE, &allocator); + ASSERT_STREQ(root_path, secure_root); +} + +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, - &allocator), - secure_root.c_str()); - 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()); + root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", + ROOT_NAMESPACE, &allocator); + ASSERT_STREQ(root_path, secure_root); } -TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride) { - rcl_allocator_t allocator = rcl_get_default_allocator(); +TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { /* 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", - &allocator), TEST_RESOURCES_DIRECTORY); + root_path = rcl_get_secure_root("name shouldn't matter", + "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", - &allocator), TEST_RESOURCES_DIRECTORY); + ASSERT_STREQ(root_path, 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( From 63458d42bb37a1eead049bbc2cd9bfb43656d4e8 Mon Sep 17 00:00:00 2001 From: Abby Xu Date: Fri, 10 May 2019 13:56:07 -0700 Subject: [PATCH 2/2] fix linter errors Signed-off-by: Abby Xu --- rcl/src/rcl/security_directory.c | 2 +- rcl/test/rcl/test_security_directory.cpp | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/rcl/src/rcl/security_directory.c b/rcl/src/rcl/security_directory.c index ab353f862..6e7d9f54e 100644 --- a/rcl/src/rcl/security_directory.c +++ b/rcl/src/rcl/security_directory.c @@ -261,4 +261,4 @@ char * rcl_get_secure_root( } allocator->deallocate(ros_secure_root_env, allocator->state); return node_secure_root; -} \ No newline at end of file +} diff --git a/rcl/test/rcl/test_security_directory.cpp b/rcl/test/rcl/test_security_directory.cpp index 806f82f82..6f2fec840 100644 --- a/rcl/test/rcl/test_security_directory.cpp +++ b/rcl/test/rcl/test_security_directory.cpp @@ -84,11 +84,11 @@ class TestGetSecureRoot : public ::testing::Test 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); + 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)); + std::min(putenv_input.length(), sizeof(g_envstring) - 1)); putenv_wrapper(g_envstring); } @@ -142,11 +142,11 @@ TEST_F(TestGetSecureRoot, successScenarios_local_prefixMatch) { * Namespace: /test_security_directory * Node: dummy_node_and_some_suffix_added */ root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", - TEST_NODE_NAMESPACE, &allocator); + TEST_NODE_NAMESPACE, &allocator); ASSERT_STRNE(root_path, secure_root); putenv_wrapper(ROS_SECURITY_LOOKUP_TYPE_VAR_NAME "=MATCH_PREFIX"); root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", - TEST_NODE_NAMESPACE, &allocator); + TEST_NODE_NAMESPACE, &allocator); ASSERT_STREQ(root_path, secure_root); } @@ -183,7 +183,7 @@ TEST_F(TestGetSecureRoot, successScenarios_root_prefixMatch) { * Namespace: / * Node: dummy_node_and_some_suffix_added */ root_path = rcl_get_secure_root(TEST_NODE_NAME "_and_some_suffix_added", - ROOT_NAMESPACE, &allocator); + ROOT_NAMESPACE, &allocator); ASSERT_STREQ(root_path, secure_root); } @@ -191,7 +191,7 @@ TEST_F(TestGetSecureRoot, nodeSecurityDirectoryOverride_validDirectory) { /* Specify a valid directory */ 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); + "namespace shouldn't matter", &allocator); ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); } @@ -200,7 +200,7 @@ TEST_F(TestGetSecureRoot, /* 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); + "namespace shouldn't matter", &allocator); putenv_wrapper(ROS_SECURITY_ROOT_DIRECTORY_VAR_NAME "=" TEST_RESOURCES_DIRECTORY); ASSERT_STREQ(root_path, TEST_RESOURCES_DIRECTORY); }