From 566e862f7091a89c6720f25a61758707e9d428d5 Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Thu, 2 Nov 2017 01:02:04 +0800 Subject: [PATCH 1/5] * Memory leak issue Failing to save or free storage allocated from rcutils_join_path(), which results in memory leak Signed-off-by: Ethan Gao --- rmw_fastrtps_cpp/src/rmw_node.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index 1a6499c42..82d700e2a 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -168,14 +168,25 @@ get_security_file_paths( const char * file_prefix = "file://"; - std::string tmpstr; + char * file_path = nullptr; for (size_t i = 0; i < num_files; i++) { - tmpstr = std::string(rcutils_join_path(node_secure_root, file_names[i])); - if (!rcutils_is_readable(tmpstr.c_str())) { - return false; + file_path = rcutils_join_path(node_secure_root, file_names[i]); + if (file_path) { + if (!rcutils_is_readable(std::string(file_path).c_str())) { + free(file_path); + file_path = nullptr; + return false; + } + + security_files_paths[i] = std::string(file_prefix + std::string(file_path)); + free(file_path); + } else { + RMW_SET_ERROR_MSG("Failed to allocate memory to get security file path"); + return false; } - security_files_paths[i] = std::string(file_prefix + tmpstr); } + + file_path = nullptr; return true; } From 10bfe5a4af60a04fa66de9c38ad0c51cdc8b2944 Mon Sep 17 00:00:00 2001 From: Ethan Gao <16472154+gaoethan@users.noreply.github.com> Date: Wed, 1 Nov 2017 21:18:44 +0800 Subject: [PATCH 2/5] remove std::string wrapper and final nullptr setting --- rmw_fastrtps_cpp/src/rmw_node.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index 82d700e2a..cc7273721 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -172,13 +172,12 @@ get_security_file_paths( for (size_t i = 0; i < num_files; i++) { file_path = rcutils_join_path(node_secure_root, file_names[i]); if (file_path) { - if (!rcutils_is_readable(std::string(file_path).c_str())) { + if (!rcutils_is_readable(file_path)) { free(file_path); - file_path = nullptr; return false; } - security_files_paths[i] = std::string(file_prefix + std::string(file_path)); + security_files_paths[i] = std::string(file_prefix + file_path); free(file_path); } else { RMW_SET_ERROR_MSG("Failed to allocate memory to get security file path"); @@ -186,7 +185,6 @@ get_security_file_paths( } } - file_path = nullptr; return true; } From d092738ad5befc9b9c23b158e939cb2e5160aa56 Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Thu, 2 Nov 2017 22:57:48 +0800 Subject: [PATCH 3/5] add back the necessary std::string for file_path and reorganize the code to make it more clear Signed-off-by: Ethan Gao --- rmw_fastrtps_cpp/src/rmw_node.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index cc7273721..8768ceef0 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -171,18 +171,20 @@ get_security_file_paths( char * file_path = nullptr; for (size_t i = 0; i < num_files; i++) { file_path = rcutils_join_path(node_secure_root, file_names[i]); - if (file_path) { - if (!rcutils_is_readable(file_path)) { - free(file_path); - return false; - } + if (!file_path) { + RMW_SET_ERROR_MSG("Failed to allocate memory for security file path"); + return false; + } - security_files_paths[i] = std::string(file_prefix + file_path); - free(file_path); + if (rcutils_is_readable(file_path)) { + security_files_paths[i] = std::string(file_prefix + std::string(file_path)); } else { - RMW_SET_ERROR_MSG("Failed to allocate memory to get security file path"); - return false; + RMW_SET_ERROR_MSG("No security file found"); + free(file_path); + return false; } + + free(file_path); } return true; From bb0cef984d5a406b8e63ea16d21735e409cbe6d8 Mon Sep 17 00:00:00 2001 From: Ethan Gao <16472154+gaoethan@users.noreply.github.com> Date: Tue, 7 Nov 2017 13:18:57 +0800 Subject: [PATCH 4/5] optimize variable scope and string converting --- rmw_fastrtps_cpp/src/rmw_node.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index 8768ceef0..e2929fe52 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -168,16 +168,15 @@ get_security_file_paths( const char * file_prefix = "file://"; - char * file_path = nullptr; for (size_t i = 0; i < num_files; i++) { - file_path = rcutils_join_path(node_secure_root, file_names[i]); + char * file_path = rcutils_join_path(node_secure_root, file_names[i]); if (!file_path) { RMW_SET_ERROR_MSG("Failed to allocate memory for security file path"); return false; } if (rcutils_is_readable(file_path)) { - security_files_paths[i] = std::string(file_prefix + std::string(file_path)); + security_files_paths[i] = std::string(file_prefix) + std::string(file_path); } else { RMW_SET_ERROR_MSG("No security file found"); free(file_path); From 15c9c556b4ce796a0e8f4f5bca09d56dbe19fca0 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 7 Nov 2017 14:37:53 +0000 Subject: [PATCH 5/5] Make file_prefix a std::string Also remove the error messages, since in the no-enforce case, we don't want random error messages to appear. Signed-off-by: Chris Lalancette --- rmw_fastrtps_cpp/src/rmw_node.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_node.cpp b/rmw_fastrtps_cpp/src/rmw_node.cpp index e2929fe52..799db7250 100644 --- a/rmw_fastrtps_cpp/src/rmw_node.cpp +++ b/rmw_fastrtps_cpp/src/rmw_node.cpp @@ -166,24 +166,22 @@ get_security_file_paths( const char * file_names[3] = {"ca.cert.pem", "cert.pem", "key.pem"}; size_t num_files = sizeof(file_names) / sizeof(char *); - const char * file_prefix = "file://"; + std::string file_prefix("file://"); for (size_t i = 0; i < num_files; i++) { - char * file_path = rcutils_join_path(node_secure_root, file_names[i]); + const char * file_path = rcutils_join_path(node_secure_root, file_names[i]); if (!file_path) { - RMW_SET_ERROR_MSG("Failed to allocate memory for security file path"); return false; } if (rcutils_is_readable(file_path)) { - security_files_paths[i] = std::string(file_prefix) + std::string(file_path); + security_files_paths[i] = file_prefix + std::string(file_path); } else { - RMW_SET_ERROR_MSG("No security file found"); - free(file_path); + free(const_cast(file_path)); return false; } - free(file_path); + free(const_cast(file_path)); } return true;