niklas/rcl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[rcl] Guard against bad allocation calling rcl_arguments_copy() (#367)

Browse source 
* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer
This commit is contained in:
Jacob Perron 2019-01-03 16:31:12 -08:00 • committed by GitHub
parent 22e61b4107
commit 65bf34b3b7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 68 additions and 32 deletions
Download patch file Download diff file Expand all files Collapse all files

70
rcl/src/rcl/arguments.c
View file

@ -499,44 +499,52 @@ rcl_arguments_copy(
// Zero so it's safe to call rcl_arguments_fini() if an error occurrs while copying. // Zero so it's safe to call rcl_arguments_fini() if an error occurrs while copying.
args_out->impl->num_remap_rules = 0; args_out->impl->num_remap_rules = 0;
args_out->impl->remap_rules = NULL;
args_out->impl->unparsed_args = NULL;
args_out->impl->num_unparsed_args = 0; args_out->impl->num_unparsed_args = 0;
args_out->impl->parameter_files = NULL;
args_out->impl->num_param_files_args = 0; args_out->impl->num_param_files_args = 0;
// Copy unparsed args if (args->impl->num_unparsed_args) {
args_out->impl->unparsed_args = allocator.allocate( // Copy unparsed args
sizeof(int) * args->impl->num_unparsed_args, allocator.state); args_out->impl->unparsed_args = allocator.allocate(
if (NULL == args_out->impl->unparsed_args) { sizeof(int) * args->impl->num_unparsed_args, allocator.state);
if (RCL_RET_OK != rcl_arguments_fini(args_out)) { if (NULL == args_out->impl->unparsed_args) {
RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
}
return RCL_RET_BAD_ALLOC;
}
for (int i = 0; i < args->impl->num_unparsed_args; ++i) {
args_out->impl->unparsed_args[i] = args->impl->unparsed_args[i];
}
args_out->impl->num_unparsed_args = args->impl->num_unparsed_args;
// Copy remap rules
args_out->impl->remap_rules = allocator.allocate(
sizeof(rcl_remap_t) * args->impl->num_remap_rules, allocator.state);
if (NULL == args_out->impl->remap_rules) {
if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
}
return RCL_RET_BAD_ALLOC;
}
args_out->impl->num_remap_rules = args->impl->num_remap_rules;
for (int i = 0; i < args->impl->num_remap_rules; ++i) {
args_out->impl->remap_rules[i] = rcl_remap_get_zero_initialized();
rcl_ret_t ret = rcl_remap_copy(
&(args->impl->remap_rules[i]), &(args_out->impl->remap_rules[i]));
if (RCL_RET_OK != ret) {
if (RCL_RET_OK != rcl_arguments_fini(args_out)) { if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error"); RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
} }
return ret; return RCL_RET_BAD_ALLOC;
}
for (int i = 0; i < args->impl->num_unparsed_args; ++i) {
args_out->impl->unparsed_args[i] = args->impl->unparsed_args[i];
}
args_out->impl->num_unparsed_args = args->impl->num_unparsed_args;
}
if (args->impl->num_remap_rules) {
// Copy remap rules
args_out->impl->remap_rules = allocator.allocate(
sizeof(rcl_remap_t) * args->impl->num_remap_rules, allocator.state);
if (NULL == args_out->impl->remap_rules) {
if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
}
return RCL_RET_BAD_ALLOC;
}
args_out->impl->num_remap_rules = args->impl->num_remap_rules;
for (int i = 0; i < args->impl->num_remap_rules; ++i) {
args_out->impl->remap_rules[i] = rcl_remap_get_zero_initialized();
rcl_ret_t ret = rcl_remap_copy(
&(args->impl->remap_rules[i]), &(args_out->impl->remap_rules[i]));
if (RCL_RET_OK != ret) {
if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
}
return ret;
}
} }
} }
// Copy parameter files // Copy parameter files
if (args->impl->num_param_files_args) { if (args->impl->num_param_files_args) {
args_out->impl->parameter_files = allocator.allocate( args_out->impl->parameter_files = allocator.allocate(
@ -558,8 +566,6 @@ rcl_arguments_copy(
return RCL_RET_BAD_ALLOC; return RCL_RET_BAD_ALLOC;
} }
} }
} else {
args_out->impl->parameter_files = NULL;
} }
return RCL_RET_OK; return RCL_RET_OK;
} }

30
rcl/test/rcl/test_arguments.cpp
View file

@ -202,6 +202,36 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_copy) {
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&copied_args)); EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&copied_args));
} }
// Similar to the default allocator, but returns NULL when size is zero.
// This is useful for emulating systems where `malloc(0)` return NULL.
// TODO(jacobperron): Consider using this allocate function in other tests
static void *
__return_null_on_zero_allocate(size_t size, void * state)
{
RCUTILS_UNUSED(state);
if (size == 0) {
return NULL;
}
return malloc(size);
}
TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_copy_no_args) {
rcl_allocator_t allocator = rcl_get_default_allocator();
allocator.allocate = __return_null_on_zero_allocate;
rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
rcl_ret_t ret = rcl_parse_arguments(0, NULL, allocator, &parsed_args);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(0, rcl_arguments_get_count_unparsed(&parsed_args));
rcl_arguments_t copied_args = rcl_get_zero_initialized_arguments();
ret = rcl_arguments_copy(&parsed_args, &copied_args);
EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
EXPECT_EQ(0, rcl_arguments_get_count_unparsed(&copied_args));
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&copied_args));
}
TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_two_namespace) { TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_two_namespace) {
const char * argv[] = {"process_name", "__ns:=/foo/bar", "__ns:=/fiz/buz"}; const char * argv[] = {"process_name", "__ns:=/foo/bar", "__ns:=/fiz/buz"};
int argc = sizeof(argv) / sizeof(const char *); int argc = sizeof(argv) / sizeof(const char *);