fix(vt-client): prevent crashes in multiple places

Also changed place of switch-case break points to align with rest of repo
Open-Agriculture · Jan 23, 2024 · 08bccba · 08bccba
1 parent 689a544
commit 08bccba
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 14 deletions.
diff --git a/isobus/include/isobus/isobus/isobus_virtual_terminal_client_state_tracker.hpp b/isobus/include/isobus/isobus/isobus_virtual_terminal_client_state_tracker.hpp
@@ -28,9 +28,15 @@ namespace isobus
 		/// @param[in] client The control function of the client. May be external.
 		explicit VirtualTerminalClientStateTracker(std::shared_ptr<ControlFunction> client);
 
+		/// @brief The destructor.
+		~VirtualTerminalClientStateTracker();
+
 		/// @brief Initializes the state tracker.
 		void initialize();
 
+		/// @brief Terminate the state tracker.
+		void terminate();
+
 		//! TODO: void initialize_with_defaults(ObjectPool &objectPool);
 
 		/// @brief Adds a numeric value to track.

diff --git a/isobus/src/isobus_virtual_terminal_client_state_tracker.cpp b/isobus/src/isobus_virtual_terminal_client_state_tracker.cpp
@@ -22,11 +22,21 @@ namespace isobus
 	{
 	}
 
+	VirtualTerminalClientStateTracker::~VirtualTerminalClientStateTracker()
+	{
+		terminate();
+	}
+
 	void VirtualTerminalClientStateTracker::initialize()
 	{
 		CANNetworkManager::CANNetwork.add_any_control_function_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::VirtualTerminalToECU), process_rx_message, this);
 	}
 
+	void VirtualTerminalClientStateTracker::terminate()
+	{
+		CANNetworkManager::CANNetwork.remove_any_control_function_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::VirtualTerminalToECU), process_rx_message, this);
+	}
+
 	void VirtualTerminalClientStateTracker::add_tracked_numeric_value(std::uint16_t objectId, std::uint32_t initialValue)
 	{
 		if (numericValueStates.find(objectId) != numericValueStates.end())
@@ -70,35 +80,44 @@ namespace isobus
 	{
 		if (message.has_valid_source_control_function() &&
 		    message.is_destination(client) &&
-		    message.is_parameter_group_number(CANLibParameterGroupNumber::VirtualTerminalToECU))
+		    message.is_parameter_group_number(CANLibParameterGroupNumber::VirtualTerminalToECU) &&
+		    (message.get_data_length() >= 1))
 		{
 			std::uint8_t function = message.get_uint8_at(0);
 			switch (function)
 			{
 				case static_cast<std::uint8_t>(VirtualTerminalClient::Function::ChangeNumericValueCommand):
 				{
-					auto errorCode = message.get_uint8_at(3);
-					if (errorCode == 0)
+					if (CAN_DATA_LENGTH == message.get_data_length())
 					{
-						std::uint16_t objectId = message.get_uint16_at(1);
-						if (numericValueStates.find(objectId) != numericValueStates.end())
+						auto errorCode = message.get_uint8_at(3);
+						if (errorCode == 0)
 						{
-							std::uint32_t value = message.get_uint32_at(4);
-							numericValueStates[objectId] = value;
+							std::uint16_t objectId = message.get_uint16_at(1);
+							if (numericValueStates.find(objectId) != numericValueStates.end())
+							{
+								std::uint32_t value = message.get_uint32_at(4);
+								numericValueStates[objectId] = value;
+							}
 						}
 					}
-					break;
 				}
+				break;
+
 				case static_cast<std::uint8_t>(VirtualTerminalClient::Function::VTChangeNumericValueMessage):
 				{
-					std::uint16_t objectId = message.get_uint16_at(1);
-					if (numericValueStates.find(objectId) != numericValueStates.end())
+					if (CAN_DATA_LENGTH == message.get_data_length())
 					{
-						std::uint32_t value = message.get_uint32_at(4);
-						numericValueStates[objectId] = value;
+						std::uint16_t objectId = message.get_uint16_at(1);
+						if (numericValueStates.find(objectId) != numericValueStates.end())
+						{
+							std::uint32_t value = message.get_uint32_at(4);
+							numericValueStates[objectId] = value;
+						}
 					}
-					break;
 				}
+				break;
+
 				default:
 					break;
 			}

diff --git a/isobus/src/isobus_virtual_terminal_client_update_helper.cpp b/isobus/src/isobus_virtual_terminal_client_update_helper.cpp
@@ -14,15 +14,25 @@
 namespace isobus
 {
 	VirtualTerminalClientUpdateHelper::VirtualTerminalClientUpdateHelper(std::shared_ptr<VirtualTerminalClient> client) :
-	  VirtualTerminalClientStateTracker(client->get_internal_control_function()),
+	  VirtualTerminalClientStateTracker(nullptr == client ? nullptr : client->get_internal_control_function()),
 	  client(client)
 	{
+		if (nullptr == client)
+		{
+			CANStackLogger::error("[VTStateHelper] constructor: client is nullptr");
+			return;
+		}
 		numericValueChangeEventHandle = client->add_vt_change_numeric_value_event_listener(
 		  std::bind(&VirtualTerminalClientUpdateHelper::process_numeric_value_change_event, this, std::placeholders::_1));
 	}
 
 	bool VirtualTerminalClientUpdateHelper::set_numeric_value(std::uint16_t object_id, std::uint32_t value)
 	{
+		if (nullptr == client)
+		{
+			CANStackLogger::error("[VTStateHelper] set_numeric_value: client is nullptr");
+			return false;
+		}
 		if (numericValueStates.find(object_id) == numericValueStates.end())
 		{
 			CANStackLogger::warn("[VTStateHelper] set_numeric_value: objectId %lu not tracked", object_id);