search

ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
534 stars 413 forks source link

allocator_common.hpp overallocates 100x memory & invokes UB on dealloc #1254

Open stevewolter opened 4 years ago

stevewolter commented 4 years ago

rclcpp/include/rclcpp/allocator/allocator_common.hpp has two severe correctness issues in memory allocation:

  1. retyped_allocate takes the "size" parameter (which is size in bytes as defined by rcl_allocator_t) and passes it to it's allocator's allocate function (which takes number of elements as its parameter). So when asked to allocate N bytes, it actually allocates N*sizeof(T) bytes, where T is the type of the allocator. It's called by subscribers and publishers with a typed allocator of type "Message", which is easily ~100 bytes, so it's overallocating memory by a factor of ~100.
    1. retyped_deallocate passes 1 to the allocator's deallocate function, instead of the size of the allocated memory. This happens to work for some allocator implementations that ignore the parameter (libstdc++'s malloc_allocator), but will lead to hard-to-diagnose segfaults on other architectures. I've observed a segfault on Linux + tcmalloc, and I guess this is the reason that allocators needed to be commented out to avoid segfaults on Windows. In any case, it's relying on undefined behaviour and a maintenance hazard.

I realize that both of these issues are not easily fixable. Since allocators are not supported yet anyway (#1061), I'd suggest to disable support for custom allocators entirely and depend on the default RCL allocator. If you concur, I'd be happy to help in implementation.

clalancette commented 4 years ago

Thanks for bringing up this issue. After reading the documentation, I agree with both of your points. I'd be OK with removing this implementation entirely and just using the default rcl allocator for now instead. But pinging @ros2/team for other opinions.

wjwwood commented 4 years ago

Yeah we can just remove it, I think it should be fixable, but I don't think anyone is really using it and we plan to move to the polymorphic allocator (we think) in the future anyways, so we can fix it then.

clalancette commented 4 years ago

Great, thanks for the feedback. @stevewolter if you are interested in working on this, we'll be happy to review. Thanks.

stevewolter commented 4 years ago

Thanks Chris, I'll give it a shot and send a PR.

stevewolter commented 4 years ago

Quick update: It will be some time - I'm on vacation for 2 weeks now, but I'll send a PR afterwards.

clalancette commented 4 years ago

@stevewolter Thanks for the heads up.

stevewolter commented 4 years ago

OK, this will take 4 commits to sort out. I'll first make get_rcl_allocator into a family of freestanding functions instead of a function template with explicit instantiation, so that current users of allocators (tlsf_cpp, demo_node) can add overrides for their custom allocators. Then I'll add these overrides, and finally delete the generic allocator implementation that causes the UB.

stevewolter commented 4 years ago

First PR https://github.com/ros2/rclcpp/pull/1324 is ready for review. Chris, could you PTAL?