Check allocator validity in some rcl_logging functions #116
Conversation
| @@ -100,6 +100,8 @@ rcl_logging_ret_t rcl_logging_external_initialize( | |||
| const char * config_file, | |||
| rcutils_allocator_t allocator) | |||
| { | |||
| RCUTILS_CHECK_ALLOCATOR(&allocator, return RCL_LOGGING_RET_ERROR); | |||
There was a problem hiding this comment.
Note that an "invalid argument" return code would be more appropriate, but this error code has already been established for at least some cases of an invalid allocator.
There was a problem hiding this comment.
the same check can be applied to rcl_logging_get_logging_directory as a function scope?
There was a problem hiding this comment.
Note that an "invalid argument" return code would be more appropriate, but this error code has already been established for at least some cases of an invalid allocator.
Doing a quick grep for RCUTILS_CHECK_ALLOCATOR through our codebase, it looks like we most often use INVALID_ARGUMENT as the return value. So I'd suggest that we switch to that here, as well as update the documentation for rcl_logging_external_initialize to say that it is a possible return code.
the same check can be applied to
rcl_logging_get_logging_directoryas a function scope?
Yeah, I think that is a good idea.
There was a problem hiding this comment.
Just so we're clear: this will technically be an API break as is demonstrated by the need to update the test.
There was a problem hiding this comment.
Just so we're clear: this will technically be an API break as is demonstrated by the need to update the test.
Yeah, I think that is OK here. This is a really low-level function, and I don't really expect anything outside of rcl to be calling it. Indeed, looking through all of the packages released into Rolling, nothing else calls it.
I also looked through some external rcl* packages:
- ros2-rust - doesn't use it
- ros2-java - doesn't use it
- ada-ros - does use it, but since the repo hasn't been updated in 3 years I think there will be other breakage as well
- r2r - doesn't use it
- rclc - doesn't use it
- rclnodejs - doesn't use it
I also did a full search on GitHub for it, and while there are a few more uses of it, I don't think anything catastrophic is going to happen if we add this return code here.
cottsay
force-pushed
the
cottsay/allocator-check
branch
from
0684cf5
to
be23bf9
Compare
If the allocator is zero-initialized, it may cause a segfault when it is used later in the functions. Signed-off-by: Scott K Logan <logans@cottsay.net>
cottsay
force-pushed
the
cottsay/allocator-check
branch
from
be23bf9
to
58f5600
Compare
cottsay
changed the title
|
|
The one thing I'm going to suggest here is that we run tests |
|
If the allocator is zero-initialized, it may cause a segfault when it is used later in the function. Right now, we're relying on other
rcutils_*functions which use the allocator to fail before we attempt to use it directly, but just because the allocator is passed to these functions doesn't always mean that it is used, especially when the program flow can change based on things like environment variables.