From 6b54f36c229eefb428fe2942626b60b6258cae85 Mon Sep 17 00:00:00 2001 From: Zard-C <58285320+Zard-C@users.noreply.github.com> Date: Fri, 28 Jul 2023 17:14:35 +0800 Subject: [PATCH 1/2] fix rclc_example: memory leaking in msg.data allocation (#386) Signed-off-by: zhangtonghe Co-authored-by: zhangtonghe (cherry picked from commit 7af20545ec3c11b2dcf1f99fc3705f10c45b6e2b) --- rclc_examples/src/example_executor.c | 2 +- rclc_examples/src/example_executor_only_rcl.c | 2 +- rclc_examples/src/example_executor_trigger.c | 3 ++- rclc_examples/src/example_pingpong.cpp | 4 ++-- rclc_examples/src/example_sub_context.c | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/rclc_examples/src/example_executor.c b/rclc_examples/src/example_executor.c index b9853ca2..bf07f566 100644 --- a/rclc_examples/src/example_executor.c +++ b/rclc_examples/src/example_executor.c @@ -109,7 +109,7 @@ int main(int argc, const char * argv[]) // assign message to publisher std_msgs__msg__String__init(&pub_msg); const unsigned int PUB_MSG_CAPACITY = 20; - pub_msg.data.data = malloc(PUB_MSG_CAPACITY); + pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state); pub_msg.data.capacity = PUB_MSG_CAPACITY; snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!"); pub_msg.data.size = strlen(pub_msg.data.data); diff --git a/rclc_examples/src/example_executor_only_rcl.c b/rclc_examples/src/example_executor_only_rcl.c index 3d1c7cf3..0b7860fc 100644 --- a/rclc_examples/src/example_executor_only_rcl.c +++ b/rclc_examples/src/example_executor_only_rcl.c @@ -127,7 +127,7 @@ int main(int argc, const char * argv[]) // assign message to publisher std_msgs__msg__String__init(&pub_msg); const unsigned int PUB_MSG_CAPACITY = 20; - pub_msg.data.data = malloc(PUB_MSG_CAPACITY); + pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state); pub_msg.data.capacity = PUB_MSG_CAPACITY; snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!"); pub_msg.data.size = strlen(pub_msg.data.data); diff --git a/rclc_examples/src/example_executor_trigger.c b/rclc_examples/src/example_executor_trigger.c index 8698bcb9..272e5166 100644 --- a/rclc_examples/src/example_executor_trigger.c +++ b/rclc_examples/src/example_executor_trigger.c @@ -139,6 +139,7 @@ void my_int_subscriber_callback(const void * msgin) void my_timer_string_callback(rcl_timer_t * timer, int64_t last_call_time) { rcl_ret_t rc; + rcl_allocator_t allocator = rcl_get_default_allocator(); RCLC_UNUSED(last_call_time); if (timer != NULL) { //printf("Timer: time since last call %d\n", (int) last_call_time); @@ -146,7 +147,7 @@ void my_timer_string_callback(rcl_timer_t * timer, int64_t last_call_time) std_msgs__msg__String pub_msg; std_msgs__msg__String__init(&pub_msg); const unsigned int PUB_MSG_CAPACITY = 20; - pub_msg.data.data = malloc(PUB_MSG_CAPACITY); + pub_msg.data.data = allocator.reallocate(pub_msg.data.data, PUB_MSG_CAPACITY, allocator.state); pub_msg.data.capacity = PUB_MSG_CAPACITY; snprintf(pub_msg.data.data, pub_msg.data.capacity, "Hello World!%d", string_pub_value++); pub_msg.data.size = strlen(pub_msg.data.data); diff --git a/rclc_examples/src/example_pingpong.cpp b/rclc_examples/src/example_pingpong.cpp index 0bba9932..0f174edf 100644 --- a/rclc_examples/src/example_pingpong.cpp +++ b/rclc_examples/src/example_pingpong.cpp @@ -181,7 +181,7 @@ int main(int argc, const char * argv[]) // assign message to publisher std_msgs__msg__String__init(&pingNode_ping_msg); const unsigned int PUB_MSG_CAPACITY = 20; - pingNode_ping_msg.data.data = (char *) malloc(PUB_MSG_CAPACITY); + pingNode_ping_msg.data.data = (char *) allocator.reallocate(pingNode_ping_msg.data.data, PUB_MSG_CAPACITY, allocator.state); pingNode_ping_msg.data.capacity = PUB_MSG_CAPACITY; snprintf(pingNode_ping_msg.data.data, pingNode_ping_msg.data.capacity, "AAAAAAAAAAAAAAAAAAA"); pingNode_ping_msg.data.size = strlen(pingNode_ping_msg.data.data); @@ -258,7 +258,7 @@ int main(int argc, const char * argv[]) // assign message to publisher std_msgs__msg__String__init(&pongNode_pong_msg); //const unsigned int PUB_MSG_CAPACITY = 20; - pongNode_pong_msg.data.data = (char *) malloc(PUB_MSG_CAPACITY); + pongNode_pong_msg.data.data = (char *) allocator.reallocate(pongNode_pong_msg.data.data, PUB_MSG_CAPACITY, allocator.state); pongNode_pong_msg.data.capacity = PUB_MSG_CAPACITY; snprintf(pongNode_pong_msg.data.data, pongNode_pong_msg.data.capacity, "BAAAAAAAAAAAAAAAAAAA"); pongNode_pong_msg.data.size = strlen(pongNode_pong_msg.data.data); diff --git a/rclc_examples/src/example_sub_context.c b/rclc_examples/src/example_sub_context.c index 86e1b22a..dc169d59 100644 --- a/rclc_examples/src/example_sub_context.c +++ b/rclc_examples/src/example_sub_context.c @@ -111,7 +111,7 @@ int main(int argc, const char * argv[]) // assign message to publisher std_msgs__msg__String__init(&( pub_msgs[i] ) ); const unsigned int PUB_MSG_CAPACITY = 40; - pub_msgs[i].data.data = malloc(PUB_MSG_CAPACITY); + pub_msgs[i].data.data = allocator.reallocate(pub_msgs[i].data.data, PUB_MSG_CAPACITY, allocator.state); pub_msgs[i].data.capacity = PUB_MSG_CAPACITY; snprintf( pub_msgs[i].data.data, pub_msgs[i].data.capacity, "Hello World! on %s", From 605a4750eecf145200c5b1d13df11d241120367f Mon Sep 17 00:00:00 2001 From: Zard-C <58285320+Zard-C@users.noreply.github.com> Date: Fri, 18 Aug 2023 16:40:37 +0800 Subject: [PATCH 2/2] fix: deprecated rcl_timer_init (#389) * fix: deprecated rcl_timer_init Signed-off-by: Zard-C Signed-off-by: zhangtonghe Signed-off-by: Zard-C * fix: switch rcl_timer_init to rcl_timer_init2 Signed-off-by: Zard-C Signed-off-by: zhangtonghe Signed-off-by: Zard-C * update rclc_timer_init_default and it's invokation Signed-off-by: zhangtonghe Signed-off-by: Zard-C * Revert "update rclc_timer_init_default and it's invokation" This reverts commit ebc3edb2482517229a07e8c6d394e36335915dd9. Signed-off-by: zhangtonghe Signed-off-by: Zard-C * fix: rcl_timer_init usage Combined changes from multiple commits to refactor the usage of rcl_timer_init for better compatibility and clarity. Signed-off-by: Zard-C Signed-off-by: zhangtonghe * Update rclc/include/rclc/timer.h Co-authored-by: Jan Staschulat Signed-off-by: Zard-C --------- Signed-off-by: Zard-C Signed-off-by: zhangtonghe Co-authored-by: zhangtonghe Co-authored-by: Jan Staschulat --- rclc/include/rclc/timer.h | 15 ++++++++++ rclc/src/rclc/timer.c | 28 +++++++++++++++---- rclc/test/rclc/test_executor.cpp | 4 +-- rclc/test/rclc/test_timer.cpp | 9 +++--- rclc_examples/src/example_executor.c | 7 +++-- rclc_examples/src/example_executor_only_rcl.c | 7 +++-- rclc_examples/src/example_executor_trigger.c | 14 ++++++---- rclc_examples/src/example_parameter_server.c | 5 ++-- rclc_examples/src/example_pingpong.cpp | 14 ++++++---- .../example_short_timer_long_subscription.c | 14 ++++++---- 10 files changed, 80 insertions(+), 37 deletions(-) diff --git a/rclc/include/rclc/timer.h b/rclc/include/rclc/timer.h index 60441529..6e8f5aee 100644 --- a/rclc/include/rclc/timer.h +++ b/rclc/include/rclc/timer.h @@ -40,11 +40,26 @@ extern "C" * \param[in] support the rclc_support_t object * \param[in] timeout_ns the time out in nanoseconds of the timer * \param[in] callback the callback of the timer + * \param[in] autostart the state of the timer at initialization * \return `RCL_RET_OK` if successful * \return `RCL_ERROR` (or other error code) if an error occurred */ RCLC_PUBLIC rcl_ret_t +rclc_timer_init_default2( + rcl_timer_t * timer, + rclc_support_t * support, + const uint64_t timeout_ns, + const rcl_timer_callback_t callback, + bool autostart); + +/** + * \deprecated `rclc_timer_init_default` implementation was removed. + * Refer to `rclc_timer_init_default2`. + */ +RCL_PUBLIC +RCUTILS_DEPRECATED_WITH_MSG("Call rclc_timer_init_default2 instead") +rcl_ret_t rclc_timer_init_default( rcl_timer_t * timer, rclc_support_t * support, diff --git a/rclc/src/rclc/timer.c b/rclc/src/rclc/timer.c index f480f9e6..57c4cee6 100644 --- a/rclc/src/rclc/timer.c +++ b/rclc/src/rclc/timer.c @@ -20,11 +20,12 @@ #include rcl_ret_t -rclc_timer_init_default( +rclc_timer_init_default2( rcl_timer_t * timer, rclc_support_t * support, const uint64_t timeout_ns, - const rcl_timer_callback_t callback) + const rcl_timer_callback_t callback, + bool autostart) { RCL_CHECK_FOR_NULL_WITH_MSG( timer, "timer is a null pointer", return RCL_RET_INVALID_ARGUMENT); @@ -32,17 +33,34 @@ rclc_timer_init_default( support, "support is a null pointer", return RCL_RET_INVALID_ARGUMENT); (*timer) = rcl_get_zero_initialized_timer(); - rcl_ret_t rc = rcl_timer_init( + rcl_ret_t rc = rcl_timer_init2( timer, &support->clock, &support->context, timeout_ns, callback, - (*support->allocator)); + (*support->allocator), + autostart); if (rc != RCL_RET_OK) { - PRINT_RCLC_ERROR(rclc_timer_init_default, rcl_timer_init); + PRINT_RCLC_ERROR(rclc_timer_init_default, rcl_timer_init2); } else { RCUTILS_LOG_INFO("Created a timer with period %ld ms.\n", timeout_ns / 1000000); } return rc; } + +rcl_ret_t +rclc_timer_init_default( + rcl_timer_t * timer, + rclc_support_t * support, + const uint64_t timeout_ns, + const rcl_timer_callback_t callback) +{ + return rclc_timer_init_default2( + timer, + support, + timeout_ns, + callback, + true + ); +} diff --git a/rclc/test/rclc/test_executor.cpp b/rclc/test/rclc/test_executor.cpp index 41201fc2..08cfbf5e 100644 --- a/rclc/test/rclc/test_executor.cpp +++ b/rclc/test/rclc/test_executor.cpp @@ -572,9 +572,9 @@ class TestDefaultExecutor : public ::testing::Test ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; this->timer1 = rcl_get_zero_initialized_timer(); ret = - rcl_timer_init( + rcl_timer_init2( &this->timer1, &this->clock, &this->context, RCL_MS_TO_NS( - this->timer1_timeout), my_timer_callback, this->clock_allocator); + this->timer1_timeout), my_timer_callback, this->clock_allocator, true); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } diff --git a/rclc/test/rclc/test_timer.cpp b/rclc/test/rclc/test_timer.cpp index ce5c8671..41e38cf6 100644 --- a/rclc/test/rclc/test_timer.cpp +++ b/rclc/test/rclc/test_timer.cpp @@ -22,7 +22,7 @@ void my_callback(rcl_timer_t * timer, int64_t last_call) RCLC_UNUSED(last_call); } -TEST(Test, rclc_timer_init_default) { +TEST(Test, rclc_timer_init_default2) { rclc_support_t support; rcl_ret_t rc; @@ -33,18 +33,19 @@ TEST(Test, rclc_timer_init_default) { const char * my_namespace = "test_namespace"; rcl_node_t node = rcl_get_zero_initialized_node(); rc = rclc_node_init_default(&node, my_name, my_namespace, &support); + bool autostart = true; // test with valid arguments rcl_timer_t timer = rcl_get_zero_initialized_timer(); - rc = rclc_timer_init_default(&timer, &support, 10000000, my_callback); + rc = rclc_timer_init_default2(&timer, &support, 10000000, my_callback, autostart); EXPECT_EQ(RCL_RET_OK, rc); // tests with invalid arguments - rc = rclc_timer_init_default(nullptr, &support, 10000000, my_callback); + rc = rclc_timer_init_default2(nullptr, &support, 10000000, my_callback, autostart); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc); rcutils_reset_error(); - rc = rclc_timer_init_default(&timer, nullptr, 10000000, my_callback); + rc = rclc_timer_init_default2(&timer, nullptr, 10000000, my_callback, autostart); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc); rcutils_reset_error(); diff --git a/rclc_examples/src/example_executor.c b/rclc_examples/src/example_executor.c index bf07f566..cf40fdaf 100644 --- a/rclc_examples/src/example_executor.c +++ b/rclc_examples/src/example_executor.c @@ -94,13 +94,14 @@ int main(int argc, const char * argv[]) // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback' rcl_timer_t my_timer = rcl_get_zero_initialized_timer(); const unsigned int timer_timeout = 1000; - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &my_timer, &support, RCL_MS_TO_NS(timer_timeout), - my_timer_callback); + my_timer_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_timeout); diff --git a/rclc_examples/src/example_executor_only_rcl.c b/rclc_examples/src/example_executor_only_rcl.c index 0b7860fc..a72202dd 100644 --- a/rclc_examples/src/example_executor_only_rcl.c +++ b/rclc_examples/src/example_executor_only_rcl.c @@ -110,15 +110,16 @@ int main(int argc, const char * argv[]) } rcl_timer_t my_timer = rcl_get_zero_initialized_timer(); const unsigned int timer_timeout = 1000; - rc = rcl_timer_init( + rc = rcl_timer_init2( &my_timer, &clock, &context, RCL_MS_TO_NS(timer_timeout), my_timer_callback, - allocator); + allocator, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init.\n"); + printf("Error in rcl_timer_init2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_timeout); diff --git a/rclc_examples/src/example_executor_trigger.c b/rclc_examples/src/example_executor_trigger.c index 272e5166..400260e4 100644 --- a/rclc_examples/src/example_executor_trigger.c +++ b/rclc_examples/src/example_executor_trigger.c @@ -225,13 +225,14 @@ int main(int argc, const char * argv[]) // - publishes 'my_string_pub' every 'timer_timeout' ms rcl_timer_t my_string_timer = rcl_get_zero_initialized_timer(); const unsigned int timer_timeout = 100; - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &my_string_timer, &support, RCL_MS_TO_NS(timer_timeout), - my_timer_string_callback); + my_timer_string_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rclc_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer 'my_string_timer' with timeout %d ms.\n", timer_timeout); @@ -257,13 +258,14 @@ int main(int argc, const char * argv[]) // - publishes 'my_int_pub' every 'timer_int_timeout' ms rcl_timer_t my_int_timer = rcl_get_zero_initialized_timer(); const unsigned int timer_int_timeout = 10 * timer_timeout; - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &my_int_timer, &support, RCL_MS_TO_NS(timer_int_timeout), - my_timer_int_callback); + my_timer_int_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rclc_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_int_timeout); diff --git a/rclc_examples/src/example_parameter_server.c b/rclc_examples/src/example_parameter_server.c index 24195f35..15209823 100644 --- a/rclc_examples/src/example_parameter_server.c +++ b/rclc_examples/src/example_parameter_server.c @@ -94,11 +94,12 @@ int main() // create timer, rcl_timer_t timer; - rclc_timer_init_default( + rclc_timer_init_default2( &timer, &support, RCL_MS_TO_NS(1000), - timer_callback); + timer_callback, + true); // Create executor // Note: diff --git a/rclc_examples/src/example_pingpong.cpp b/rclc_examples/src/example_pingpong.cpp index 0f174edf..69fdfc79 100644 --- a/rclc_examples/src/example_pingpong.cpp +++ b/rclc_examples/src/example_pingpong.cpp @@ -166,13 +166,14 @@ int main(int argc, const char * argv[]) // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback' rcl_timer_t ping_timer ; const unsigned int timer_timeout = 50; //50 milliseconds userdefined value - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &ping_timer, &support, RCL_MS_TO_NS(timer_timeout), - ping_timer_callback); + ping_timer_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_timeout); @@ -243,13 +244,14 @@ int main(int argc, const char * argv[]) // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback' rcl_timer_t pong_timer ; const unsigned int pong_timer_timeout = 50; //50 milliseconds userdefined value - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &pong_timer, &support, RCL_MS_TO_NS(pong_timer_timeout), - pong_timer_callback); + pong_timer_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_timeout); diff --git a/rclc_examples/src/example_short_timer_long_subscription.c b/rclc_examples/src/example_short_timer_long_subscription.c index 1cefce47..746b0374 100644 --- a/rclc_examples/src/example_short_timer_long_subscription.c +++ b/rclc_examples/src/example_short_timer_long_subscription.c @@ -108,13 +108,14 @@ int main(int argc, const char * argv[]) // create a timer, which will call the publisher with period=`timer_timeout` ms in the 'my_timer_callback' rcl_timer_t my_timer = rcl_get_zero_initialized_timer(); const unsigned int timer_timeout = 1000; - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &my_timer, &support, RCL_MS_TO_NS(timer_timeout), - my_timer_callback); + my_timer_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", timer_timeout); @@ -122,13 +123,14 @@ int main(int argc, const char * argv[]) rcl_timer_t short_timer = rcl_get_zero_initialized_timer(); const unsigned int short_timer_timeout = 100; - rc = rclc_timer_init_default( + rc = rclc_timer_init_default2( &short_timer, &support, RCL_MS_TO_NS(short_timer_timeout), - short_timer_callback); + short_timer_callback, + true); if (rc != RCL_RET_OK) { - printf("Error in rcl_timer_init_default.\n"); + printf("Error in rclc_timer_init_default2.\n"); return -1; } else { printf("Created timer with timeout %d ms.\n", short_timer_timeout);