From 814ceb21184b8b7dbb3070fb063b0f248aec0b96 Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Mon, 11 Dec 2017 23:24:03 +0800 Subject: [PATCH 1/3] Invalid memory access for NULL dereference rcl_lifecycle_get_state may return NULL transition->goal is checked for NULL but no return while it's true and this may result the follow-up NULL dereference Signed-off-by: Ethan Gao --- rcl_lifecycle/src/rcl_lifecycle.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rcl_lifecycle/src/rcl_lifecycle.c b/rcl_lifecycle/src/rcl_lifecycle.c index 6c9e30b6b..80bf50598 100644 --- a/rcl_lifecycle/src/rcl_lifecycle.c +++ b/rcl_lifecycle/src/rcl_lifecycle.c @@ -285,15 +285,22 @@ rcl_lifecycle_is_valid_transition( const rcl_lifecycle_state_t * current_state = rcl_lifecycle_get_state( &state_machine->transition_map, current_id); + if (NULL == current_state) { + RCL_SET_ERROR_MSG("rcl_lifecycle_get_state returns NULL", rcl_get_default_allocator()); + return NULL; + } + for (unsigned int i = 0; i < current_state->valid_transition_size; ++i) { if (current_state->valid_transition_keys[i] == key) { return ¤t_state->valid_transitions[i]; } } + RCUTILS_LOG_WARN_NAMED( ROS_PACKAGE_NAME, "No callback transition matching %d found for current state %s", key, state_machine->current_state->label) + return NULL; } @@ -318,6 +325,7 @@ rcl_lifecycle_trigger_transition( if (!transition->goal) { RCUTILS_LOG_ERROR_NAMED( ROS_PACKAGE_NAME, "No valid goal is set") + return RCL_RET_ERROR; } state_machine->current_state = transition->goal; if (publish_notification) { From 61a2eb260623e58bd7b31859d55acbd7d7e3ea4a Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Tue, 12 Dec 2017 02:15:00 +0800 Subject: [PATCH 2/3] Avoid crash while NULL returned from rcl_service_get_options() Signed-off-by: Ethan Gao --- rcl/src/rcl/service.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 2df5b0125..e9800cfe0 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -247,6 +247,8 @@ rcl_take_request( { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Service server taking service request") const rcl_service_options_t * options = rcl_service_get_options(service); + RCL_CHECK_FOR_NULL_WITH_MSG( + options, "Failed to get service option", return RCL_RET_ERROR, rcl_get_default_allocator()); RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT, options->allocator); RCL_CHECK_ARGUMENT_FOR_NULL(ros_request, RCL_RET_INVALID_ARGUMENT, options->allocator); @@ -273,6 +275,8 @@ rcl_send_response( { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Sending service response") const rcl_service_options_t * options = rcl_service_get_options(service); + RCL_CHECK_FOR_NULL_WITH_MSG( + options, "Failed to get service option", return RCL_RET_ERROR, rcl_get_default_allocator()); RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT, options->allocator); RCL_CHECK_ARGUMENT_FOR_NULL(ros_response, RCL_RET_INVALID_ARGUMENT, options->allocator); From bb986de3aab9bdd7be76f2066e6368dc36b508c8 Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Wed, 13 Dec 2017 21:09:32 +0800 Subject: [PATCH 3/3] tweak error string and null check for current_state Signed-off-by: Ethan Gao --- rcl/src/rcl/service.c | 4 ++-- rcl_lifecycle/src/rcl_lifecycle.c | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index e9800cfe0..a3c606f53 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -248,7 +248,7 @@ rcl_take_request( RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Service server taking service request") const rcl_service_options_t * options = rcl_service_get_options(service); RCL_CHECK_FOR_NULL_WITH_MSG( - options, "Failed to get service option", return RCL_RET_ERROR, rcl_get_default_allocator()); + options, "Failed to get service options", return RCL_RET_ERROR, rcl_get_default_allocator()); RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT, options->allocator); RCL_CHECK_ARGUMENT_FOR_NULL(ros_request, RCL_RET_INVALID_ARGUMENT, options->allocator); @@ -276,7 +276,7 @@ rcl_send_response( RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Sending service response") const rcl_service_options_t * options = rcl_service_get_options(service); RCL_CHECK_FOR_NULL_WITH_MSG( - options, "Failed to get service option", return RCL_RET_ERROR, rcl_get_default_allocator()); + options, "Failed to get service options", return RCL_RET_ERROR, rcl_get_default_allocator()); RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT, options->allocator); RCL_CHECK_ARGUMENT_FOR_NULL(ros_response, RCL_RET_INVALID_ARGUMENT, options->allocator); diff --git a/rcl_lifecycle/src/rcl_lifecycle.c b/rcl_lifecycle/src/rcl_lifecycle.c index 80bf50598..824610132 100644 --- a/rcl_lifecycle/src/rcl_lifecycle.c +++ b/rcl_lifecycle/src/rcl_lifecycle.c @@ -285,10 +285,8 @@ rcl_lifecycle_is_valid_transition( const rcl_lifecycle_state_t * current_state = rcl_lifecycle_get_state( &state_machine->transition_map, current_id); - if (NULL == current_state) { - RCL_SET_ERROR_MSG("rcl_lifecycle_get_state returns NULL", rcl_get_default_allocator()); - return NULL; - } + RCL_CHECK_FOR_NULL_WITH_MSG(current_state, + "rcl_lifecycle_get_state returns NULL", return NULL, rcl_get_default_allocator()); for (unsigned int i = 0; i < current_state->valid_transition_size; ++i) { if (current_state->valid_transition_keys[i] == key) {