Fix out of bounds read in CoreUnsafeUtils.FixedBufferStringQueue

Author: arttu-peltonen

Packages/com.unity.render-pipelines.core/Runtime/Common/CoreUnsafeUtils.cs

@@ -69,12 +69,18 @@ public bool TryPush(string v)
             }
 
             /// <summary>
-            /// Pop an element of the queue.
+            /// Try to pop an element of the queue.
             /// </summary>
             /// <param name="v">Output result string.</param>
-            /// <returns>True if an element was succesfuly poped.</returns>
+            /// <returns>True if an element was successfully popped.</returns>
             public bool TryPop(out string v)
             {
+                if (m_ReadCursor + sizeof(int) >= m_BufferEnd)
+                {
+                    v = null;
+                    return false;
+                }
+
                 var size = *(int*)m_ReadCursor;
                 if (size != 0)
                 {
@@ -84,7 +90,7 @@ public bool TryPop(out string v)
                     return true;
                 }
 
-                v = default;
+                v = null;
                 return false;
             }
 

Packages/com.unity.render-pipelines.core/Tests/Editor/FixedBufferStringQueueTests.cs

@@ -36,6 +36,31 @@ public void PushAndPopOutOfBufferRange()
             Assert.False(buffer.TryPush("amet, consectetur adipiscing"));
 
             Assert.AreEqual(1, buffer.Count);
+
+            Assert.True(buffer.TryPop(out string v) && v == "Lorem ipsum dolor sit");
+            Assert.False(buffer.TryPop(out v) && v == null);
+        }
+
+        [Test]
+        public void PushAndPopOutOfBufferRange_StringSizeNotDivisibleBy4()
+        {
+            // UUM-104687: Buffer is created with size of 32 bytes. The test fills the first 30 bytes with a string,
+            // so 2 bytes are left over in the buffer. After we pop the string out, we check that the next TryPop
+            // doesn't try to read out of bounds when trying to read the string length.
+
+            const int bufferLength = 32;
+            const int bytesToFill = bufferLength - 2;
+            const int bytesForString = bytesToFill - sizeof(int);
+            const int numCharacters = bytesForString / sizeof(char);
+
+            string testValue = new string('a', numCharacters);
+
+            byte* bufferStart = stackalloc byte[bufferLength];
+            CoreUnsafeUtils.FixedBufferStringQueue buffer = new CoreUnsafeUtils.FixedBufferStringQueue(bufferStart, bufferLength);
+
+            Assume.That(buffer.TryPush(testValue));
+            Assume.That(buffer.TryPop(out string v));
+            Assert.False(buffer.TryPop(out v));
         }
 
         [Test]
@@ -58,5 +83,6 @@ public void PushAndPopAndClear()
             Assert.True(buffer.TryPop(out string v) && v == "elit, sed do eiusmod");
             Assert.True(buffer.TryPop(out v) && v == "tempor incididunt ut labore");
         }
+
     }
 }