-
Notifications
You must be signed in to change notification settings - Fork 418
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
Fix the dereference to NULL #405
Conversation
|
||
const char * validation_result_string = rmw_node_name_validation_result_string( | ||
validation_result); | ||
validation_result_string = validation_result_string ? validation_result_string : ""; |
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.
While using the ternary operator isn't exactly wrong, it is definitely odd to be re-assigning a variable back to itself. I'd prefer a simple if (nullptr == validation_result_string)
here. Also, we should probably have a slightly different string than ""
; maybe something like "Unknown error"
.
The same goes for the rest of the calls below.
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.
actually, I've ever tried to make the error string specific, but when the result is valid, it also return NULL which is different from the default NULL in some sense, so it's not accurate if we treat all the NULL to "unknown error", I prefer to tweak the rmw_*validate_result_string
to get it more accurate, any comments ? thanks.
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.
Yeah, I think I agree that we should ensure that the rmw_*_validation_result_string
should probably never return NULL. I'll make a similar comment over in ros2/rmw#130 , but let's leave this open for now just in case we come to a different conclusion over there.
please evaluate to unify the undefined case in rmw_*_validation_result_string or deal with separate code while using based on #ros2/rmw#130 |
} else { | ||
using rclcpp::exceptions::InvalidTopicNameError; | ||
throw InvalidTopicNameError(name.c_str(), validation_message, invalid_index); | ||
eles { |
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.
typo ?
This also need to be on the same line as the above closing curly bracket to pass the linters
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.
Also this else is not necessary as if we enter the if we throw an error so the previous code can be kept
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.
yes, it's a typo (my bad), actually the else got changed to not on the same line after I ran ament_uncrusitify --reformat, let me check.
why I add the else is because it will return NULL
if validation_result
is RCL_TOPIC_NAME_VALID
, it will be problematic for InvalidTopicNameError
and InvalidServiceNameError
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.
why I add the else is because it will return NULL if validation_result is RCL_TOPIC_NAME_VALID, it will be problematic for InvalidTopicNameError and InvalidServiceNameError
My understanding is that this is taken care of by the if just before
if (validation_result == RCL_TOPIC_NAME_VALID) {
throw std::runtime_error("topic name unexpectedly valid");
}
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.
@gaoethan This comment has not been addressed.
I think that the following snippet should be kept. If this evaluates to true the code below is unreacheable as we throw. If it doesnt we're sure we're not in the case of validation_result == RCL_TOPIC_NAME_VALID
if (validation_result == RCL_TOPIC_NAME_VALID) {
throw std::runtime_error("topic name unexpectedly valid");
}
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.
Actually, I have kept it, :) generally speaking, it has lower possibility to hit that and I put it in the else statement
node_name.c_str(), | ||
rmw_node_name_validation_result_string(validation_result), | ||
invalid_index); | ||
} |
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.
Not sure if what scenario that can happen but should we do something in the else case ? (case where the rcl node name is valid but the rmw node name is not) (same everywhere below)
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.
this if
is under umbrella of RCL_RET_NODE_INVALID_NAME
and it's currently handling the cases of invalid rmw node name as you point :). or what you really mean the case that rcl node name is invalid but rwm node name is valid ?
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.
Indeed that's what I meant 😄
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.
@mikaelarguedas now it's not sure when this will happen, I throw exceptions for the case of valid rmw with invalid rcl checking to allow the user to be aware of that for further investigation.
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.
except for one remaining comment this looks good to me
rmw_*_validation_result_string(validation_result) may return NULL, and it's dereferenced by passing arg to NameValidationError Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
to the change of API rmw_*_validation_result_string Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
lots of things changed
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.
lgtm
* Update after launch_testing features becoming legacy. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Migrate rcl tests to new launch_testing API. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Migrate missing rcl test to new launch_testing API. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
…ncludes are not required (ros2#405) Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
rmw_*_validation_result_string(validation_result)
may return NULL,and it's dereferenced by passing arg to
NameValidationError
, thisleads unexpected behaviors and NULL is deprecated to use in C++ now.
Signed-off-by: Ethan Gao ethan.gao@linux.intel.com
Connects to ros2/rmw#130