Skip to content

Commit

Permalink
fix memory leak in test_security_directory
Browse files Browse the repository at this point in the history 
Signed-off-by: Abby Xu <abbyxu@amazon.com>
  • Loading branch information
@xabxx
xabxx committed May 8, 2019
1 parent 0c24c7e commit 40fb8cb
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 54 deletions.
36 changes: 21 additions & 15 deletions rcl/src/rcl/security_directory.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
}
1 change: 1 addition & 0 deletions rcl/test/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
128 changes: 89 additions & 39 deletions rcl/test/rcl/test_security_directory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);

Expand All @@ -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
Expand All @@ -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)
Expand All @@ -97,65 +141,71 @@ 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
* --------------------------
* 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
* --------------------------
* 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(
Expand Down

0 comments on commit 40fb8cb

Please sign in to comment.