code that should segfault?

[VRichardJP]

Hi

I am looking at node.c and arguments.c code, and there is part that, in my understanding, should cause a segfault (but I guess it doesn't?)

If you look from line 286 (node.c in master branch):

  ...
  node->impl->options = rcl_node_get_default_options();
  node->context = context;
  // Initialize node impl.
  ret = rcl_node_options_copy(options, &(node->impl->options));
  ...

node->impl->options is filled with 0, and then it is copied to options with the following function (from line 562):

  ...
  options_out->domain_id = options->domain_id;
  options_out->allocator = options->allocator;
  options_out->use_global_arguments = options->use_global_arguments;
  if (NULL != options->arguments.impl) {
    rcl_ret_t ret = rcl_arguments_copy(&(options->arguments), &(options_out->arguments));
    ...

and the arguments are copied by rcl_arguments_copy as defined in arguments.c from line 479:

  ...
  args_out->impl = allocator.allocate(sizeof(rcl_arguments_impl_t), allocator.state);
  if (NULL == args_out->impl) {
    return RCL_RET_BAD_ALLOC;
  }

  args_out->impl->allocator = allocator;

  // 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_unparsed_args = 0;
  args_out->impl->num_param_files_args = 0;

  // Copy unparsed args
  args_out->impl->unparsed_args = allocator.allocate(
    sizeof(int) * args->impl->num_unparsed_args, allocator.state);
  if (NULL == args_out->impl->unparsed_args) {
    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;
  }
  ...

So args_out->impl is allocated. Since the allocation here is just a malloc, args_out->impl
may contain any kind of value.

Since at this point args->impl->num_unparsed_args is 0, this line:

  allocator.allocate(
    sizeof(int) * args->impl->num_unparsed_args, allocator.state);

is just a malloc(0), which returns NULL. So the if statement is triggered and we call rcl_arguments_fini(args_out) (from line 567). At this point args->impl->remap_rules may contain anything (it has not been initialized yet), so the code may then enter the if statement at line 574:

    if (args->impl->remap_rules) {
      ...
      args->impl->allocator.deallocate(args->impl->remap_rules, args->impl->allocator.state);
      ...
}

and then call free on the uninitialized pointer, resulting in a SegFault

Is my understanding correct?

[jacobperron]

Since at this point args->impl->num_unparsed_args is 0, this line:

I believe you are referring to this statement:

rcl/rcl/src/rcl/arguments.c

Lines 505 to 507 in 6b6c0fe

	// Copy unparsed args
	args_out->impl->unparsed_args = allocator.allocate(
	sizeof(int) * args->impl->num_unparsed_args, allocator.state);

But, args->impl->num_unparsed_args is not necessarily zero (note it is the input parameter to rcl_argument_copy, not the output parameter). Although, I suppose it could be zero if when the original arguments are parsed, no arguments are passed:

rcl/rcl/src/rcl/arguments.c

Lines 227 to 230 in 6b6c0fe

	if (argc == 0) {
	// there are no arguments to parse
	return RCL_RET_OK;
	}

We can try to produce a possible issue with a test.

[jacobperron]

I tried adding the following test to test/rcl/test_arguments.cpp and it passes for me:

TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_copy_no_args) {
  rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
  rcl_ret_t ret = rcl_parse_arguments(0, NULL, rcl_get_default_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));
}

[jacobperron]

I'm still not convinced there's not an issue here though. The behavior of malloc(0) is documented as implementation defined (ref):

If size is zero, the behavior is implementation defined (null pointer may be returned, or some non-null pointer may be returned that may not be used to access storage, but has to be passed to free).

It's probably not a bad idea to commit the test and add a guard against allocating/deallocating unparsed_args.

[jacobperron]

Based on this SO post and some empirical testing, it seems we don't have to worry about free'ing the pointer returned by malloc(0).

@vingtfranc In summary, to try and answer your original concern, in the event we have 0 unparsed arguments, the resulting malloc(0) may return NULL, and if it does, a call to free(NULL) during finalize is safe according to the standard ISO/IEC 9899 Section 7.20.3.2:

The free function causes the space pointed to by ptr to be deallocated, that is, made
available for further allocation. If ptr is a null pointer, no action occurs.

If I've misunderstood something and there is still a problem, we can reopen this issue.

[VRichardJP]

Based on this SO post and some empirical testing, it seems we don't have to worry about free'ing the pointer returned by malloc(0).

@vingtfranc In summary, to try and answer your original concern, in the event we have 0 unparsed arguments, the resulting malloc(0) may return NULL, and if it does, a call to free(NULL) during finalize is safe according to the standard ISO/IEC 9899 Section 7.20.3.2:

The free function causes the space pointed to by ptr to be deallocated, that is, made
available for further allocation. If ptr is a null pointer, no action occurs.

If I've misunderstood something and there is still a problem, we can reopen this issue.

Yes, but in the case I'm describing, it is not the malloc(0) which is freed:

rcl/rcl/src/rcl/arguments.c

Lines 505 to 513 in 6b6c0fe

	// Copy unparsed args
	args_out->impl->unparsed_args = allocator.allocate(
	sizeof(int) * args->impl->num_unparsed_args, allocator.state);
	if (NULL == args_out->impl->unparsed_args) {
	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;
	}

Here if malloc(0) returns NULL (it does on my system), then args_out->impl->unparsed_args is NULL but args_out->impl->remap_rules can be anything since there is just a malloc on args_out->impl

rcl/rcl/src/rcl/arguments.c

Line 493 in 6b6c0fe

args_out->impl = allocator.allocate(sizeof(rcl_arguments_impl_t), allocator.state);

Thus, when rcl_arguments_fini(args_out) is called, args_out->impl->remap_rules contains a random value, which is misinterpreted as a pointer, and freed:

rcl/rcl/src/rcl/arguments.c

Lines 568 to 584 in 6b6c0fe

	rcl_arguments_fini(
	rcl_arguments_t * args)
	{
	RCL_CHECK_ARGUMENT_FOR_NULL(args, RCL_RET_INVALID_ARGUMENT);
	if (args->impl) {
	rcl_ret_t ret = RCL_RET_OK;
	if (args->impl->remap_rules) {
	for (int i = 0; i < args->impl->num_remap_rules; ++i) {
	rcl_ret_t remap_ret = rcl_remap_fini(&(args->impl->remap_rules[i]));
	if (remap_ret != RCL_RET_OK) {
	ret = remap_ret;
	RCUTILS_LOG_ERROR_NAMED(
	ROS_PACKAGE_NAME,
	"Failed to finalize remap rule while finalizing arguments. Continuing...");
	}
	}
	args->impl->allocator.deallocate(args->impl->remap_rules, args->impl->allocator.state);

Besides, even if there was no segfault here, rcl_arguments_copy would still raise a RCL_RET_BAD_ALLOC in the case malloc(0) returns NULL.

[jacobperron]

@vingtfranc Ah, I see the issue now. So are you saying this doesn't segfault for you? Perhaps it was just "luck" that in this case args_out->impl->remap_rules was zero.

Looks like a bug to me.

[VRichardJP]

Actually, I have compiled the demo node listener.cpp on my experimental system, and the program has the behavior I described (checked step-by-step in gdb). Note that I'm not building ROS 2 the standard way (as my machine has its own building system). So it's possible the case I'm describing may only occur because of my building configuration.

On my side, I fixed my code modifying allocator.c:
https://github.com/ros2/rcutils/blob/3630b909c4dd1e459f1cc13f258b1357501e20bc/src/allocator.c#L31

static void *
__default_allocate(size_t size, void * state)
{
  RCUTILS_UNUSED(state);
  return malloc(size);
}

into

static void *
__default_allocate(size_t size, void * state)
{
  RCUTILS_UNUSED(state);
#ifdef __myarch__
if (size == 0) return malloc(1);
#endif 
  return malloc(size);
}

What does happen on your machine if you add a if (size == 0) return NULL in the allocate?

[jacobperron]

What does happen on your machine if you add a if (size == 0) return NULL in the allocate?

The test I suggested in #362 (comment) fails due to bad allocation and test_remap_integration fails (crashes).

It would appear a few places in our code do not anticipate the possibility that malloc(0) returns NULL.

[jacobperron]

@vingtfranc Please take a look at #367. I believe it addresses this particular issue.
Thank you for reporting this issue 🙂