memory leak issue

[gaoethan]

Failing to save or free storage allocated from
rcutils_join_path(), which results in memory leak

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

[clalancette]

Good catch, this is definitely a memory leak. I've got a couple of suggestions to simplify the code, but once those are fixed I think I'm good with this change.

[clalancette]

There's no real reason to wrap this in a std::string, and then get the c_str out of it; file_path is already a C-style string. Just do if (!rcutils_is_readable(file_path)) {

[gaoethan]

it makes sense, I initially kept it as the original :) and now it's revised

[clalancette]

There's no need to set file_path to nullptr here; it is a stack variable, so is going out of scope immediately. It probably doesn't matter for the final code (I'm assuming the compiler will optimize this away), but static analyzers will probably pick this up as a statement that has no effect.

[gaoethan]

generally speaking, it's safer with specified manually, but that's not mandatory for modern compiler now, removing is oaky as well 😄 , thanks.

[clalancette]

There's no need to set file_path to nullptr here; it is a stack variable, so is going out of scope immediately. It probably doesn't matter for the final code (I'm assuming the compiler will optimize this away), but static analyzers will probably pick this up as a statement that has no effect. The same below at the final return in the function.

[clalancette]

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 ca.cert.pem, but fail to find cert.pem. In that case, we'll have allocated memory for ca.cert.pem, failed to find cert.pem, freed the memory for cert.pem, then returned false. At which point create_node will assume the entire function failed, and we'll still leak the memory for ca.cert.pem. I think we'll probably want to make the contract for get_security_file_paths such that they all succeed, or none of them do. In that case, I think we need to unwind any allocations we did in previous iterations of the loop here. Does that make sense?

[gaoethan]

I don't think so, you can see the memory allocated for ca.cert.pem has been freed if it's found from the following snippet:

security_files_paths[i] = std::string(file_prefix + file_path);
free(file_path);

So even it fails to find cert.pem, it has nothing to do with ca.cert.pem, because they use their memory allocated respectively. right ?

[clalancette]

Yeah, you are correct. Sorry, I was thinking about it incorrectly. This part is fine.

[clalancette]

Oh, also, I'm pretty sure file_prefix + file_path is wrong here. That's going to add two pointers, not concatenate the two of them together.

[gaoethan]

exactly, the std::string should keep for file_path here

[wjwwood]

Could you not reduce the scope of the variable to within the for loop?

[clalancette]

While this works, it is faster to make file_prefix above a std::string (like std::string file_prefix("file://");, and then here, do:

security_file_paths[i] = file_prefix + std::string(file_path);

Once we have that, and reduce the scope of the file_path like @wjwwood suggested, I think this will be ready to go.

[clalancette]

I ended up pushing a few fixes to the code. In particular, my push did:

1. Made file_prefix a std::string, getting rid of some copies in the loop
2. Made the thing compile by doing a const_cast during the free
3. Got rid of all of the RMW_SET_ERROR. The reason for this is a bit subtle. If get_security_file_paths fails, then iff security_options->enforce_security is true, we want to send an error. However, rmw_create_node (the caller of get_security_file_paths) already sets an error message, so setting it in get_security_file_paths is unnecessary work. In the case where security_options->enforce_security is false, we don't want to send any errors out of rmw_create_node (at least, not for this reason). Therefore, I think it is probably inappropriate to set errors down in get_security_file_paths at all.

I'm going to run CI on this now. @wjwwood , if you have a minute to review my work on top of Ethan's, that would be appreciated.

[clalancette]

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

Really, rcutils_join_path should not return const char * but just char * instead of needing to const cast it to free the result...

[clalancette]

Yeah, I agree with that. I'll do a separate PR with that, since every place that uses it has to do the same thing.

[clalancette]

ros2/ros2#423

[clalancette]

The CI test failures are because of the known flake8 problem, reported here: ros2/build_farmer#67. Otherwise, this is looking pretty good. I'm going to merge this up. Thanks for the work!