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 rclc_example: memory leaking in msg.data allocation (backport #386) #387

Merged
merged 2 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions rclc/include/rclc/timer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
28 changes: 23 additions & 5 deletions rclc/src/rclc/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,47 @@
#include <rcutils/logging_macros.h>

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);
RCL_CHECK_FOR_NULL_WITH_MSG(
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
);
}
4 changes: 2 additions & 2 deletions rclc/test/rclc/test_executor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
9 changes: 5 additions & 4 deletions rclc/test/rclc/test_timer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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();

Expand Down
9 changes: 5 additions & 4 deletions rclc_examples/src/example_executor.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -109,7 +110,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);
Expand Down
9 changes: 5 additions & 4 deletions rclc_examples/src/example_executor_only_rcl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -127,7 +128,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);
Expand Down
17 changes: 10 additions & 7 deletions rclc_examples/src/example_executor_trigger.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,15 @@ 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);

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);
Expand Down Expand Up @@ -224,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);
Expand All @@ -256,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);
Expand Down
5 changes: 3 additions & 2 deletions rclc_examples/src/example_parameter_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
18 changes: 10 additions & 8 deletions rclc_examples/src/example_pingpong.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -181,7 +182,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);
Expand Down Expand Up @@ -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);
Expand All @@ -258,7 +260,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);
Expand Down
14 changes: 8 additions & 6 deletions rclc_examples/src/example_short_timer_long_subscription.c
Original file line number Diff line number Diff line change
Expand Up @@ -108,27 +108,29 @@ 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);
}

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);
Expand Down
2 changes: 1 addition & 1 deletion rclc_examples/src/example_sub_context.c
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down