Double freeing memory, writing memory after freeing, and memory leak in RMW custom allocations

[rjp5th]

Describe the Bug

The initialization of the custom session memory (the memory allocated by the rmw get_memory routine) appears to have allocated/freed in a way that if a step fails during the initialization routine, the memory may not be freed at all, it may attempt to free the memory twice, or modify the contents of the allocated memory after it has been freed. This appears to be present in several of the custom session memory allocations (publishers, subscribers, services, clients, and possibly others).

The following issues were observed:

1. Memory Double Free:

   For example in rmw_create_publisher, during the last run_xrce_session call before returning (rmw_publisher.c:211 in commit 40c175), if this fails for whatever reason (say the agent shuts down), the memory will be freed with put_memory. This will continue on to call rmw_uxrce_fini_publisher_memory after jumping to the fail label, which will in turn call put_memory again as rmw_publisher->data had been assigned to custom_publisher.

2. Memory modified after freeing:

   For example in rmw_create_subscription, during the first call to run_xrce_session (rmw_subscription.c:158 in commit 40c175), if this call fails the memory will be freed with put_memory. The logic will continue to the fail label, where rmw_uxrce_fini_subscription_memory will be called. At this point it will attempt to read implementation_identifier of rmw_subscription, even though custom_subscription which contains rmw_subscription has already been freed. If this value is non-NULL, it will be overwritten to NULL by this routine, even though it is no longer the owner of this memory.

3. Memory Leak:

   For example in rmw_create_service, during the checking of the service name string length, if the string is too long it will jump to fail and call rmw_uxrce_fini_service_memory. However, since rmw_service->data is not assigned until later in the function, put_memory is never called on the custom_service object, resulting in a memory leak. This will cause issues later during cleanup of the node as implementation_identifier is NULL, yet it will attempt to destroy this service rmw_destroy_service with the service handle is not from this implementation.

Note that these are just one example of each of these situations, but there appears to be many branches which could cause the memory allocations to not be handled properly. I have also not looked into other memory allocations such as the node or wait set memory to see if they have the same issue.

One final note is I found that during the rmw_destroy_node function when it is looping through and destroying all publisher, subscription, service, and client memory allocations it will overwrite the return value, meaning that the return value will only show the result of the last destroy call, whatever object that may be.

System information:

• OS: Ubuntu 22.04
• ROS 2 Humble
• Version 40c175
• Platform: micro_ros_raspberrypi_pico_sdk

[pablogs9]

Thanks a lot for the report, we will take a look.

[Acuadros95]

Hi @rjp5th,

I have created a PR https://github.com/micro-ROS/rmw_microxrcedds/pull/280 solving those issues. The other creation methods have also been checked, and everything looks OK.

PTAL and give feedback on the fix.

[rjp5th]

I just tested with the PR mentioned above and the issue is resolved, thank you!