[rcl_yaml_param_parser] Add warnings #831
Conversation
|
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
The changes seem reasonable to me, but please make sure to get the upgraded googletest PR merged and released into Rolling before merging. That way we won't break PR jobs for all future PRs.
|
|
|
@ros-pull-request-builder retest this, please. |
Just FYI, the buildfarm is still rebuilding things (it turns out that googletest causes almost all packages to rebuild, so it takes a while). So you may just have to wait until tomorrow morning for the builds to finish and be synced to testing. |
|
@ros-pull-request-builder retest this, please. |
|
It looks like the remaining errors on the PR job are actually warnings from |
audrow
force-pushed
the
audrow/add-warnings-rcl_yaml_param_parser
branch
from
7e4111e to
4b07655
Compare
Signed-off-by: Audrow Nash <[email protected]>
audrow
force-pushed
the
audrow/add-warnings-rcl_yaml_param_parser
branch
from
4b07655 to
21a1d46
Compare
Signed-off-by: Audrow Nash <[email protected]>
audrow
force-pushed
the
audrow/add-warnings-rcl_yaml_param_parser
branch
from
1a9eb2d to
0e3978c
Compare
|
| } else { | ||
| do { | ||
| size_t len = separator_pos - absolute_namespace - i; | ||
| size_t len = ((size_t) (separator_pos - absolute_namespace)) - i; |
There was a problem hiding this comment.
I know this code isn't new, but this case is a little scary. In particular, are we sure that (separator_pos - absolute_namespace) is always >= i? If not, we can get into a situation where we pass a very large number into rcutils_strndup, leading to bad behavior. Would you mind looking into this a bit and seeing what you can find out?
There was a problem hiding this comment.
I believe that separator_pos - absolute_namespace is always >= i. I tried to make a proof-like explanation that showed it, but it's hard to do in markdown and probably more confusing than anything.
Basically, i is the sum of the previous lengths of namespaces already processed. The separator_pos - absolute_namespace is used to effectively get the index (through pointer arithmetic) of the next separator character (/). i is then used to skip the already considered part of absolute_namespace, so I should always be less than or equal to the index obtained from separator_pos - absolute_namespace.
Here is a simplified version of what's going on: https://repl.it/@audrow/Test-validating-namespace.
There was a problem hiding this comment.
That works for me. Thanks for looking into it.
| } else { | ||
| do { | ||
| size_t len = separator_pos - absolute_namespace - i; | ||
| size_t len = ((size_t) (separator_pos - absolute_namespace)) - i; |
There was a problem hiding this comment.
That works for me. Thanks for looking into it.
delete-merged-branch
bot
deleted the
audrow/add-warnings-rcl_yaml_param_parser
branch
This PR enables
-Wformat=2,-Wconversion, and-Wsign-conversioninrcl_yaml_param_parser. No source code was modified to enable these warnings. This PR relies on using gtest v1.10.0, see ament/googletest#8.