Fix behavior when provided a malformed parameters file#556
Fix behavior when provided a malformed parameters file#556jacobperron merged 5 commits intoeloquentfrom
Conversation
The function assumes that the structure has already been initialized, so it should be the callers responsibility to finalize it. Otherwise, this may lead to a double free as reported in #553. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This restores behavior that was lost when the '__params:=' syntax was deprecated in #495. An extra guard was also added when finalizing the parameters struct. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
clalancette
left a comment
There was a problem hiding this comment.
I've got one suggestion for a comment improvement. The code looks good to me otherwise.
Can you add a test that tickles this problem?
rcl/src/rcl/arguments.c
Outdated
| // If we return RCL_RET_ERROR then the argument contained the prefix '__params:=', | ||
| // but the parameter file may be malformed or does not exist. |
There was a problem hiding this comment.
I'd suggest something like:
// If _rcl_parse_param_file_rule() returned RCL_RET_ERROR, then the argument contained the '__params:=' prefix,
// but parsing of the parameter file failed. Get out early here.
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
@clalancette I've added a test that fails without this fix. PTAL. 98433bc |
hidmic
left a comment
There was a problem hiding this comment.
Looks good overall, thanks for taking care @jacobperron !
clalancette
left a comment
There was a problem hiding this comment.
Looks good with green CI.
Also warn about deprecated usage, even if there is an error parsing parameters file. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
hidmic
left a comment
There was a problem hiding this comment.
LGTM! And test failures on MacOS seem unrelated.
|
@jacobperron I may be misunderstanding this, but I only see this commit on the Eloquent branch, not the master branch. If that is the case, we need to port it over to the master branch as well. |
|
This commit was only intended for the Eloquent branch. The bug was introduced when the old ROS CLI arguments were deprecated (see #553 (comment)). Since the deprecated syntax has since been removed on master, there is nothing to fix. |
Ah! I forgot about that, I was just noticing commits only on this branch. Thanks for the reminder. |
Fixes #553.
The function assumes that the structure has already been initialized, so it should be the callers
responsibility to finalize it. Otherwise, this may lead to a double free as reported in SIGABRT parsing yaml config file (double free detected) #553.
This restores behavior that was lost when the '__params:=' syntax was deprecated in Promote special CLI rules to flags #495.