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

[stevewolter]

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.
2. 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]

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]

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]

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

[stevewolter]

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

[stevewolter]

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

[clalancette]

@stevewolter Thanks for the heads up.