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

Fix memory allocation in rclc_parameter #413

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

bjsowa
Copy link
Contributor

@bjsowa bjsowa commented Dec 7, 2024

  1. Fixed rclc_parameter_descriptor_initialize_string null dereference after failed allocation.
  2. Simplified rclc_parameter_server_fini_memory function. There is no need to call fini on all fields of the message as the top-level fini function will do this either way.
  3. Rewritten init functions to return on first error. This fixes issue with init_parameter_server_memory where it tries to dereference an object after it failed to allocate.

I haven't touched the low memory mode but I'm fairly certain it can use the same fini memory function.

@bjsowa bjsowa force-pushed the rlrc-parameter-fix-memory-allocation branch from cc34818 to 06f5f2c Compare December 7, 2024 02:14
Copy link
Contributor

Choose a reason for hiding this comment

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

While CHECK_BOOL_RET is a concise and effective macro for error handling, I don't think it's a good idea, ts use in the codebase should be carefully considered. Inconsistent usage, lack of flexibility, and the potential for debugging difficulties or hidden side effects can cause maintainability issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any alternatives suggestions?

@@ -360,6 +360,8 @@ rcl_ret_t rclc_parameter_server_init_default(
parameter_server, node, &DEFAULT_PARAMETER_SERVER_OPTIONS);
}

#define CHECK_BOOL_RET(ret) do {if (!(ret)) {return RCL_RET_ERROR;}} while (0)
Copy link
Contributor

Choose a reason for hiding this comment

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

can be more flexible in future use?

Suggested change
#define CHECK_BOOL_RET(ret) do {if (!(ret)) {return RCL_RET_ERROR;}} while (0)
#define CHECK_BOOL_RET(ret, error_type) do {if (!(ret)) {return error_type;}} while (0)

Comment on lines +764 to +765
#define CHECK_RCL_RET(ret) do {rcl_ret_t temp_ret = (ret); \
if (temp_ret != RCL_RET_OK) {return temp_ret;}} while (0)
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not really fan of this macro, i would inline those code instead of introducing macro.

i think you are trying to avoid adding those error check code, but there are many other places. so that should be just fine. and we could also add debug logging information as well.

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