-
Notifications
You must be signed in to change notification settings - Fork 117
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.
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
memory leak issue #172
memory leak issue #172
Changes from 2 commits
566e862
10bfe5a
d092738
bb0cef9
15c9c55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,14 +168,23 @@ 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(file_path)) { | ||
free(file_path); | ||
return false; | ||
} | ||
|
||
security_files_paths[i] = std::string(file_prefix + file_path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, also, I'm pretty sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly, the |
||
free(file_path); | ||
} else { | ||
RMW_SET_ERROR_MSG("Failed to allocate memory to get security file path"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I didn't notice this before, but I think we still have a leak here. Consider the case where we successfully find and allocate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, you can see the memory allocated for
So even it fails to find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you are correct. Sorry, I was thinking about it incorrectly. This part is fine. |
||
return false; | ||
} | ||
security_files_paths[i] = std::string(file_prefix + tmpstr); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you not reduce the scope of the variable to within the for loop?