Fixing conditional jump based on uninitialized optional values #48
Conversation
topic_tools/src/tool_base_node.cpp
Outdated
| @@ -31,7 +31,7 @@ void ToolBaseNode::make_subscribe_unsubscribe_decisions() | |||
| { | |||
| if (auto source_info = try_discover_source()) { | |||
| // always relay same topic type and QoS profile as the first available source | |||
| if (*topic_type_ != source_info->first || *qos_profile_ != source_info->second || !pub_) { | |||
| if (!topic_type_ || !qos_profile_ || *topic_type_ != source_info->first || *qos_profile_ != source_info->second || !pub_) { | |||
There was a problem hiding this comment.
Or || operators in if statement doesn't guarantee order of operations for processor.
Processor can execute *topic_type_ != source_info->first || *qos_profile_ != source_info->second || !pub_ and than check !topic_type_ || !qos_profile_ It seems fix doesn't fully address the problem.
There was a problem hiding this comment.
Thanks for your reply, @MichaelOrlov. Aren't logical OR as well as AND operators exceptions to the rule of unspecified order of evaluation in an expression? According to cppreference.com: Every value computation and side effect of the first (left) argument of the built-in logical AND operator && and the built-in logical OR operator || is sequenced before every value computation and side effect of the second (right) argument. Therefore, should any of our optionals here be uninitialized, the || operator will short-circuit and won't evaluate its following arguments.
There was a problem hiding this comment.
@bartek-kc Never mind, it seems after C++ 11 things got better and sequence of the or operations for || respecting as well.
There was a problem hiding this comment.
@bartek-kc Please address linters warnings and sign your PR.
At least uncrustify complain about
+++ topic_tools/src/tool_base_node.cpp.uncrustify
@@ -34 +34,3 @@
- if (!topic_type_ || !qos_profile_ || *topic_type_ != source_info->first || *qos_profile_ != source_info->second || !pub_) {
+ if (!topic_type_ || !qos_profile_ || *topic_type_ != source_info->first ||
+ *qos_profile_ != source_info->second || !pub_)
+ {
Signed-off-by: bartek-kc <bartosz.kozlowiec@robotec.ai>
Signed-off-by: bartek-kc <bartosz.kozlowiec@robotec.ai>
bartek-kc
force-pushed
the
std_optional_usage_fix
branch
from
166909b to
20c0191
Compare
bartek-kc
requested review from
MichaelOrlov
and removed request for
emersonknapp and
christophebedard
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/e9fdae3db05615033ea0a6e76be88b88/raw/f236f5a9d60c0b35f8e5b75b5050d709b41df425/ros2.repos
|
|
Warnings on Windows build are unrelated to the changes in this PR and was existing before. |
This pull request aims to fix a potentially dangerous usage of
std::optionalin which a conditional jump is made based on uninitialized value (or values). The fix eliminates the following error message issued by Valgrind: "Conditional jump or move depends on uninitialised value(s)". Quoting cppreference.com:std::optional<T>::operator*does not check whether the optional contains a value!