{"action":"created","issue":{"url":"https://api.github.com/repos/ros2/rcl/issues/362","repository_url":"https://api.github.com/repos/ros2/rcl","labels_url":"https://api.github.com/repos/ros2/rcl/issues/362/labels{/name}","comments_url":"https://api.github.com/repos/ros2/rcl/issues/362/comments","events_url":"https://api.github.com/repos/ros2/rcl/issues/362/events","html_url":"https://github.com/ros2/rcl/issues/362","id":392082547,"node_id":"MDU6SXNzdWUzOTIwODI1NDc=","number":362,"title":"code that should segfault?","user":{"login":"vingtfranc","id":18645627,"node_id":"MDQ6VXNlcjE4NjQ1NjI3","avatar_url":"https://avatars0.githubusercontent.com/u/18645627?v=4","gravatar_id":"","url":"https://api.github.com/users/vingtfranc","html_url":"https://github.com/vingtfranc","followers_url":"https://api.github.com/users/vingtfranc/followers","following_url":"https://api.github.com/users/vingtfranc/following{/other_user}","gists_url":"https://api.github.com/users/vingtfranc/gists{/gist_id}","starred_url":"https://api.github.com/users/vingtfranc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vingtfranc/subscriptions","organizations_url":"https://api.github.com/users/vingtfranc/orgs","repos_url":"https://api.github.com/users/vingtfranc/repos","events_url":"https://api.github.com/users/vingtfranc/events{/privacy}","received_events_url":"https://api.github.com/users/vingtfranc/received_events","type":"User","site_admin":false},"labels":[{"id":179183675,"node_id":"MDU6TGFiZWwxNzkxODM2NzU=","url":"https://api.github.com/repos/ros2/rcl/labels/bug","name":"bug","color":"d73a4a","default":true}],"state":"open","locked":false,"assignee":{"login":"jacobperron","id":1218431,"node_id":"MDQ6VXNlcjEyMTg0MzE=","avatar_url":"https://avatars1.githubusercontent.com/u/1218431?v=4","gravatar_id":"","url":"https://api.github.com/users/jacobperron","html_url":"https://github.com/jacobperron","followers_url":"https://api.github.com/users/jacobperron/followers","following_url":"https://api.github.com/users/jacobperron/following{/other_user}","gists_url":"https://api.github.com/users/jacobperron/gists{/gist_id}","starred_url":"https://api.github.com/users/jacobperron/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jacobperron/subscriptions","organizations_url":"https://api.github.com/users/jacobperron/orgs","repos_url":"https://api.github.com/users/jacobperron/repos","events_url":"https://api.github.com/users/jacobperron/events{/privacy}","received_events_url":"https://api.github.com/users/jacobperron/received_events","type":"User","site_admin":false},"assignees":[{"login":"jacobperron","id":1218431,"node_id":"MDQ6VXNlcjEyMTg0MzE=","avatar_url":"https://avatars1.githubusercontent.com/u/1218431?v=4","gravatar_id":"","url":"https://api.github.com/users/jacobperron","html_url":"https://github.com/jacobperron","followers_url":"https://api.github.com/users/jacobperron/followers","following_url":"https://api.github.com/users/jacobperron/following{/other_user}","gists_url":"https://api.github.com/users/jacobperron/gists{/gist_id}","starred_url":"https://api.github.com/users/jacobperron/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jacobperron/subscriptions","organizations_url":"https://api.github.com/users/jacobperron/orgs","repos_url":"https://api.github.com/users/jacobperron/repos","events_url":"https://api.github.com/users/jacobperron/events{/privacy}","received_events_url":"https://api.github.com/users/jacobperron/received_events","type":"User","site_admin":false}],"milestone":null,"comments":6,"created_at":"2018-12-18T10:10:14Z","updated_at":"2018-12-22T00:14:10Z","closed_at":null,"author_association":"NONE","body":"Hi\r\n\r\nI 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?)\r\n\r\nIf you look from line 286 (`node.c` in master branch):\r\n```\r\n  ...\r\n  node->impl->options = rcl_node_get_default_options();\r\n  node->context = context;\r\n  // Initialize node impl.\r\n  ret = rcl_node_options_copy(options, &(node->impl->options));\r\n  ...\r\n```\r\n`node->impl->options` is filled with 0, and then it is copied to `options` with the following function (from line 562):\r\n```\r\n  ...\r\n  options_out->domain_id = options->domain_id;\r\n  options_out->allocator = options->allocator;\r\n  options_out->use_global_arguments = options->use_global_arguments;\r\n  if (NULL != options->arguments.impl) {\r\n    rcl_ret_t ret = rcl_arguments_copy(&(options->arguments), &(options_out->arguments));\r\n    ...\r\n```\r\n\r\nand the arguments are copied by `rcl_arguments_copy` as defined in `arguments.c` from line 479:\r\n```\r\n  ...\r\n  args_out->impl = allocator.allocate(sizeof(rcl_arguments_impl_t), allocator.state);\r\n  if (NULL == args_out->impl) {\r\n    return RCL_RET_BAD_ALLOC;\r\n  }\r\n\r\n  args_out->impl->allocator = allocator;\r\n\r\n  // Zero so it's safe to call rcl_arguments_fini() if an error occurrs while copying.\r\n  args_out->impl->num_remap_rules = 0;\r\n  args_out->impl->num_unparsed_args = 0;\r\n  args_out->impl->num_param_files_args = 0;\r\n\r\n  // Copy unparsed args\r\n  args_out->impl->unparsed_args = allocator.allocate(\r\n    sizeof(int) * args->impl->num_unparsed_args, allocator.state);\r\n  if (NULL == args_out->impl->unparsed_args) {\r\n    if (RCL_RET_OK != rcl_arguments_fini(args_out)) {\r\n      RCL_SET_ERROR_MSG(\"Error while finalizing arguments due to another error\");\r\n    }\r\n    return RCL_RET_BAD_ALLOC;\r\n  }\r\n  ...\r\n```\r\n\r\nSo `args_out->impl` is allocated. Since the allocation here is just a malloc, `args_out->impl`\r\n may contain any kind of value. \r\n\r\nSince at this point `args->impl->num_unparsed_args` is 0, this line:\r\n```\r\n  allocator.allocate(\r\n    sizeof(int) * args->impl->num_unparsed_args, allocator.state);\r\n```\r\nis 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:\r\n```\r\n    if (args->impl->remap_rules) {\r\n      ...\r\n      args->impl->allocator.deallocate(args->impl->remap_rules, args->impl->allocator.state);\r\n      ...\r\n}\r\n```\r\nand then call `free` on the uninitialized pointer, resulting in a SegFault\r\n\r\nIs my understanding correct?"},"comment":{"url":"https://api.github.com/repos/ros2/rcl/issues/comments/449528540","html_url":"https://github.com/ros2/rcl/issues/362#issuecomment-449528540","issue_url":"https://api.github.com/repos/ros2/rcl/issues/362","id":449528540,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0OTUyODU0MA==","user":{"login":"vingtfranc","id":18645627,"node_id":"MDQ6VXNlcjE4NjQ1NjI3","avatar_url":"https://avatars0.githubusercontent.com/u/18645627?v=4","gravatar_id":"","url":"https://api.github.com/users/vingtfranc","html_url":"https://github.com/vingtfranc","followers_url":"https://api.github.com/users/vingtfranc/followers","following_url":"https://api.github.com/users/vingtfranc/following{/other_user}","gists_url":"https://api.github.com/users/vingtfranc/gists{/gist_id}","starred_url":"https://api.github.com/users/vingtfranc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vingtfranc/subscriptions","organizations_url":"https://api.github.com/users/vingtfranc/orgs","repos_url":"https://api.github.com/users/vingtfranc/repos","events_url":"https://api.github.com/users/vingtfranc/events{/privacy}","received_events_url":"https://api.github.com/users/vingtfranc/received_events","type":"User","site_admin":false},"created_at":"2018-12-22T00:14:10Z","updated_at":"2018-12-22T00:14:10Z","author_association":"NONE","body":"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.\r\n\r\nOn my side, I fixed my code modifying allocator.c:\r\nhttps://github.com/ros2/rcutils/blob/3630b909c4dd1e459f1cc13f258b1357501e20bc/src/allocator.c#L31\r\n```\r\nstatic void *\r\n__default_allocate(size_t size, void * state)\r\n{\r\n  RCUTILS_UNUSED(state);\r\n  return malloc(size);\r\n}\r\n```\r\ninto\r\n```\r\nstatic void *\r\n__default_allocate(size_t size, void * state)\r\n{\r\n  RCUTILS_UNUSED(state);\r\n#ifdef __myarch__\r\nif (size == 0) return malloc(1);\r\n#endif \r\n  return malloc(size);\r\n}\r\n```\r\n\r\nWhat does happen on your machine if you add a `if (size == 0) return NULL` in the allocate?"}}
{"actor":{"display_login":"vingtfranc"}}