Skip to content

Commit

Permalink
Add debug-level asserts for poor time or configuration to prevent blo…
Browse files Browse the repository at this point in the history
…cking in uxr_create_session

* Added asserts that protect against time going backwards or staying static
* Added information to header on a call being blocking and for how long
* Increased const-correctecness of time handling
* Add assert for handling narrowing conversion of time
* Add CMake protection for incorrectly configuring options that can block for unreasonable amount of time
* CMake should theoretically remove the asserts in release mode, so there should be no overhead
* Reference: https://stackoverflow.com/questions/34302265/does-cmake-build-type-release-imply-dndebug
* Closes eProsima#350

Signed-off-by: Ryan Friedman <[email protected]>
  • Loading branch information
Ryanf55 committed Mar 4, 2023
1 parent bc6e9c3 commit 4e1b7b5
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ set(UCLIENT_MAX_INPUT_BEST_EFFORT_STREAMS 1 CACHE STRING "Set the maximum number
set(UCLIENT_MAX_INPUT_RELIABLE_STREAMS 1 CACHE STRING "Set the maximum number of input reliable streams for session.")
set(UCLIENT_MAX_SESSION_CONNECTION_ATTEMPTS 10 CACHE STRING "Set the number of connection attemps.")
set(UCLIENT_MIN_SESSION_CONNECTION_INTERVAL 1000 CACHE STRING "Set the connection interval in milliseconds.")
if(${UCLIENT_MIN_SESSION_CONNECTION_INTERVAL} LESS_EQUAL 0)
message(FATAL_ERROR "UCLIENT_MIN_SESSION_CONNECTION_INTERVAL is out of range")
endif()
set(UCLIENT_MIN_HEARTBEAT_TIME_INTERVAL 100 CACHE STRING "Set the time interval between heartbeats in milliseconds.")
set(UCLIENT_UDP_TRANSPORT_MTU 512 CACHE STRING "Set the UDP transport MTU.")
set(UCLIENT_TCP_TRANSPORT_MTU 512 CACHE STRING "Set the TCP transport MTU.")
Expand Down
3 changes: 3 additions & 0 deletions include/uxr/client/core/session/session.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ UXRDLLAPI void uxr_set_performance_callback(
/**
* @brief Creates a new session with the Agent.
* This function logs in a session, enabling any other XRCE communication with the Agent.
* It blocks for a time proportional to
* (UXR_CONFIG_MAX_SESSION_CONNECTION_ATTEMPTS * UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL)
* until a response from the Agent is received.
* @param session A uxrSesssion structure previously initialized.
* @return true in case of successful session establishment, and false in other case.
*/
Expand Down
17 changes: 13 additions & 4 deletions src/c/core/session/session.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
#include <uxr/client/profile/multithread/multithread.h>
#include "../../profile/shared_memory/shared_memory_internal.h"

#include <assert.h>
#include <limits.h>

#ifdef UCLIENT_PROFILE_SHARED_MEMORY
#define PROFILE_SHARED_MEMORY_ADD_SIZE 21
#else
Expand Down Expand Up @@ -749,13 +752,19 @@ bool wait_session_status(
{
send_message(session, buffer, length);

int64_t start_timestamp = uxr_millis();
int remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL;
const int64_t start_timestamp = uxr_millis();
int64_t remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL;
assert(remaining_time > 0);

do
{
listen_message(session, remaining_time);
remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL - (int)(uxr_millis() - start_timestamp);
assert(remaining_time <= INT_MAX); // Protect the narrowing conversion
listen_message(session, (int)remaining_time);
const int64_t current_timestamp = uxr_millis();
assert(current_timestamp >= start_timestamp);
const int64_t elapsed_time = current_timestamp - start_timestamp;
assert(UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL >= elapsed_time);
remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL - elapsed_time;
} while (remaining_time > 0 && session->info.last_requested_status == UXR_STATUS_NONE);
}

Expand Down

0 comments on commit 4e1b7b5

Please sign in to comment.