Fix potiential nullptr dereferenced issue in demo_nodes_cpp #242
Conversation
There was a problem hiding this comment.
Thanks for the PR, @testkit. The comments that I've written apply to all of the files that have been changed, but also there are some files where the usage has not been updated e.g.
. Could you update those too please?| @@ -65,9 +65,15 @@ int main(int argc, char ** argv) | |||
| } | |||
|
|
|||
| auto topic = std::string("add_two_ints"); | |||
|
|
|||
| if (rcutils_cli_option_exist(argv, argv + argc, "-s")) { | |||
There was a problem hiding this comment.
we might as well remove this check now that we do not assume that rcutils_cli_get_option will return a valid string.
| topic = std::string(rcutils_cli_get_option(argv, argv + argc, "-s")); | ||
| char * cli_option = rcutils_cli_get_option(argv, argv + argc, "-s"); | ||
|
|
||
| if (NULL != cli_option) { |
There was a problem hiding this comment.
please use nullptr instead of NULL in C++
| if (rcutils_cli_option_exist(argv, argv + argc, "-s")) { | ||
| topic = std::string(rcutils_cli_get_option(argv, argv + argc, "-s")); | ||
| char * cli_option = rcutils_cli_get_option(argv, argv + argc, "-s"); | ||
|
|
There was a problem hiding this comment.
please remove the blank space between these lines as they are directly related
dhood
added
in progress
testkit
force-pushed
the
Fix_nullptr_dereference_issue
branch
from
8553141
to
ae9c989
Compare
|
@dhood, thanks for your review. PR updated as below: |
mikaelarguedas
added
in review
Edit based on comments about nullptr, blank space, redundent rcutils_cli_option_exist() calling. Fix the similar bug in topics/listener.cpp, topics/talker.cpp
dhood
force-pushed
the
Fix_nullptr_dereference_issue
branch
from
ae9c989
to
09b635b
Compare
|
I fixed the style errors. @testkit in the future please be sure to run tests for your code changes because we also have tests that check that the code complies with our style guidelines.
|
|
@dhood , noted and thanks! |
Need check if it's nullptr before do std::string() convert.