Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix memory leaks in test_security_directory #420

Merged
merged 2 commits into from
May 14, 2019
Merged

fix memory leaks in test_security_directory #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b06f0f
fix memory leak in test_security_directory
xabxx May 8, 2019
63458d4
fix linter errors
xabxx May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions rcl/src/rcl/security_directory.c
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 "=";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified IMHO, what about using rcutils_format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mempy on line 93 needs a size, return of rcutils_format_string is a char *. sticking with using std:string here to get length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get the length with strlen.

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