ros2 / rcutils

Common C functions and data structures used in ROS 2
Apache License 2.0
56 stars 100 forks source link

validate the allocator before use. #455

Closed fujitatomoya closed 5 months ago

fujitatomoya commented 6 months ago

minor patch to avoid possible segmentation fault in rcutils function scope.

related to https://github.com/ros2/rcl_logging/pull/116

fujitatomoya commented 6 months ago

CC: @clalancette @cottsay

cottsay commented 6 months ago

A few thoughts:

  1. I don't think we should check this value in a function unless the function specifically uses it. If it just passes it through to another function, that other function should be responsible for checking it.
  2. This is pretty nitpicky, but IIRC the convention is/was to check arguments for validity after checking for the presence of required arguments (i.e. null checks before value checks)
  3. In general, I support more argument checking like this, but these are pretty low-level functions and they do get called pretty often. I don't think we'll see any noticeable performance impact, but we should be mindful.
fujitatomoya commented 6 months ago

note:

fujitatomoya commented 5 months ago

@ahcorde can you take a look at this?

ahcorde commented 5 months ago