allocator_common.hpp overallocates 100x memory & invokes UB on dealloc
Open
@wjwwood is already working on this.
Since Feb 3, 2022.
help wanted
- Dominant language
- C++
- Stars
- 805
- Forks
- 564
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 27
Description
rclcpp/include/rclcpp/allocator/allocator_common.hpp has two severe correctness issues in memory allocation:
- 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.
- 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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.