Skip to content

[rcl_yaml_param_parser] Add warnings #831

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 4, 2020
Merged

[rcl_yaml_param_parser] Add warnings #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21a1d46
Add warnings
audrow Oct 13, 2020
0e3978c
Fix conversion warning by casting to size_t
audrow Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion rcl_yaml_param_parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ if(NOT CMAKE_CXX_STANDARD)
endif()

if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic)
add_compile_options(
-Wall -Wextra -Wpedantic
-Wformat=2 -Wconversion -Wsign-conversion
)
endif()

set(rcl_yaml_parser_sources
Expand Down
4 changes: 2 additions & 2 deletions rcl_yaml_param_parser/src/parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ _validate_name(const char * name, rcutils_allocator_t allocator)
}
} else {
// substring namespace including the last '/'
char * namespace_ = rcutils_strndup(name, separator_pos - name + 1, allocator);
char * namespace_ = rcutils_strndup(name, ((size_t) (separator_pos - name)) + 1, allocator);
if (NULL == namespace_) {
ret = RCUTILS_RET_BAD_ALLOC;
goto clean;
Expand Down Expand Up @@ -534,7 +534,7 @@ _validate_name(const char * name, rcutils_allocator_t allocator)
}
} else {
do {
size_t len = separator_pos - absolute_namespace - i;
size_t len = ((size_t) (separator_pos - absolute_namespace)) - i;
Copy link
Contributor

@clalancette clalancette Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@audrow audrow Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll look into it.

Copy link
Member Author

@audrow audrow Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@clalancette clalancette Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me. Thanks for looking into it.

char * namespace_ = rcutils_strndup(absolute_namespace + i, len, allocator);
if (NULL == namespace_) {
ret = RCUTILS_RET_BAD_ALLOC;
Expand Down