Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate the allocator before use. #455

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

fujitatomoya
Copy link
Collaborator

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

related to ros2/rcl_logging#116

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

CC: @clalancette @cottsay

@cottsay
Copy link
Member

cottsay commented Mar 6, 2024

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.

- validate allocator only if the function specifically uses.
- argument null check comes before validation of value.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cottsay thank you for your comment #455 (comment). i fixed the code accordingly.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Mar 14, 2024

note:

  • although we do not have particular performance profiler, some places are considered as performance concern. if the check is during initialization and finalization, that should be no problem.
  • assertion debug macro would be useful during the test to replace those checks.
  • for performance sensitive execution paths, we can remove the check and add it to the doc section, allocate must not be null, otherwise unexpected behavior.
  • if the allocator comes from implementation, there is no need to check that.

@fujitatomoya fujitatomoya self-assigned this Mar 14, 2024
@fujitatomoya
Copy link
Collaborator Author

@ahcorde can you take a look at this?

@ahcorde
Copy link
Contributor

ahcorde commented Apr 5, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit e67a4ad into rolling Apr 5, 2024
2 of 4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/validate-allocator-before-use branch April 5, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants