fix: handle null or empty named message registration [MTT-7771] (#2807)

* fix Fixes issue where registering or unregistering named message that is null or empty does not throw an exception. Fixes issue where it was possible that NetworkTransform could try to unregister a named message before it had been assigned a value. * update Actually decided to make both cases log an error and raised the minimum loglevel. * test Adding a test to verify that trying to register or unregister a null or empty named message will not throw an exception. * style removing whitespace * update Adding change log entries
Unity-Technologies · Jan 13, 2024 · 64ed674 · 64ed674
1 parent 073f861
commit 64ed674
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 1 deletion.
diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md
@@ -6,6 +6,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com).
 
+## [Unreleased]
+
+### Added
+
+### Fixed
+
+- Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807)
+
+### Changed
+
+- Changed `CustomMessageManager` so it no longer attempts to register or "unregister" a null or empty string and will log an error if this condition occurs. (#2807)
+
+
 ## [1.8.0] - 2023-12-12
 
 ### Added

diff --git a/com.unity.netcode.gameobjects/Components/NetworkTransform.cs b/com.unity.netcode.gameobjects/Components/NetworkTransform.cs
@@ -2763,7 +2763,7 @@ public override void OnNetworkSpawn()
         public override void OnNetworkDespawn()
         {
             // During destroy, use NetworkBehaviour.NetworkManager as opposed to m_CachedNetworkManager
-            if (!NetworkManager.ShutdownInProgress && NetworkManager.CustomMessagingManager != null)
+            if (!NetworkManager.ShutdownInProgress && NetworkManager.CustomMessagingManager != null && !string.IsNullOrEmpty(m_MessageName))
             {
                 NetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(m_MessageName);
             }

diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/CustomMessageManager.cs
@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using Unity.Collections;
+using UnityEngine;
 
 namespace Unity.Netcode
 {
@@ -199,6 +200,14 @@ internal void InvokeNamedMessage(ulong hash, ulong sender, FastBufferReader read
         /// <param name="callback">The callback to run when a named message is received.</param>
         public void RegisterNamedMessageHandler(string name, HandleNamedMessageDelegate callback)
         {
+            if (string.IsNullOrEmpty(name))
+            {
+                if (m_NetworkManager.LogLevel <= LogLevel.Error)
+                {
+                    Debug.LogError($"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!");
+                }
+                return;
+            }
             var hash32 = XXHash.Hash32(name);
             var hash64 = XXHash.Hash64(name);
 
@@ -215,6 +224,15 @@ public void RegisterNamedMessageHandler(string name, HandleNamedMessageDelegate
         /// <param name="name">The name of the message.</param>
         public void UnregisterNamedMessageHandler(string name)
         {
+            if (string.IsNullOrEmpty(name))
+            {
+                if (m_NetworkManager.LogLevel <= LogLevel.Error)
+                {
+                    Debug.LogError($"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!");
+                }
+                return;
+            }
+
             var hash32 = XXHash.Hash32(name);
             var hash64 = XXHash.Hash64(name);
 

diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Messaging/NamedMessageTests.cs
@@ -86,6 +86,24 @@ public void NamedMessageIsReceivedOnHostWithContent()
             Assert.AreEqual(m_ServerNetworkManager.LocalClientId, receivedMessageSender);
         }
 
+        private void MockNamedMessageCallback(ulong sender, FastBufferReader reader)
+        {
+
+        }
+
+        [Test]
+        public void NullOrEmptyNamedMessageDoesNotThrowException()
+        {
+            LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!");
+            m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(string.Empty, MockNamedMessageCallback);
+            LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.RegisterNamedMessageHandler)}] Cannot register a named message of type null or empty!");
+            m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(null, MockNamedMessageCallback);
+            LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!");
+            m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(string.Empty);
+            LogAssert.Expect(UnityEngine.LogType.Error, $"[{nameof(CustomMessagingManager.UnregisterNamedMessageHandler)}] Cannot unregister a named message of type null or empty!");
+            m_ServerNetworkManager.CustomMessagingManager.UnregisterNamedMessageHandler(null);
+        }
+
         [UnityTest]
         public IEnumerator NamedMessageIsReceivedOnMultipleClientsWithContent()
         {