[Upstream] Fix out of bounds access in dtMergeCorridorStartMoved and add tests

size can become negative if req > maxPath. This may happen when visited buffer is larger than path buffer.
Add tests to cover different use cases of the function including Should add visited points not present in path up to the path capacity to cover the fix.
List tests files explicitly. When new file is added CMake does not add it to the already generated list if GLOB is used.

- 599fd0f023
- https://github.com/recastnavigation/recastnavigation/pull/635

replace comment in DtPathCorridor

checking

tt

npath

src/DotRecast.Core/Buffers/RcRentedArray.cs

@@ -6,7 +6,7 @@ namespace DotRecast.Core.Buffers
 {
     public static class RcRentedArray
     {
-        public static RcRentedArray<T> Rent<T>(int minimumLength)
+        public static RcRentedArray<T> RentDisposableArray<T>(int minimumLength)
         {
             var array = ArrayPool<T>.Shared.Rent(minimumLength);
             return new RcRentedArray<T>(ArrayPool<T>.Shared, array, minimumLength);
@@ -17,41 +17,44 @@ namespace DotRecast.Core.Buffers
     {
         private ArrayPool<T> _owner;
         private T[] _array;
+        private readonly RcAtomicInteger _disposed;
 
         public int Length { get; }
-        public bool IsDisposed => null == _owner || null == _array;
 
         internal RcRentedArray(ArrayPool<T> owner, T[] array, int length)
         {
             _owner = owner;
             _array = array;
             Length = length;
+            _disposed = new RcAtomicInteger(0);
         }
 
-        public ref T this[int index]
+        public T this[int index]
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             get
             {
                 RcThrowHelper.ThrowExceptionIfIndexOutOfRange(index, Length);
-                return ref _array[index];
+                return _array[index];
             }
-        }
 
-        public T[] AsArray()
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        {
+            set
-            return _array;
+            {
+                RcThrowHelper.ThrowExceptionIfIndexOutOfRange(index, Length);
+                _array[index] = value;
+            }
         }
 
 
         public void Dispose()
         {
-            if (null != _owner && null != _array)
+            if (1 != _disposed.IncrementAndGet())
-            {
+                return;
-                _owner.Return(_array, true);
+
-                _owner = null;
+            _owner?.Return(_array, true);
-                _array = null;
+            _array = null;
-            }
+            _owner = null;
         }
     }
 }

src/DotRecast.Detour/DtPathUtils.cs

@@ -176,6 +176,7 @@ namespace DotRecast.Detour
 
             // Adjust beginning of the buffer to include the visited.
             List<long> result = new List<long>();
+            
             // Store visited
             for (int i = visited.Count - 1; i > furthestVisited; --i)
             {

test/DotRecast.Core.Test/RcArrayBenchmarkTests.cs

@@ -1,101 +0,0 @@
-using System;
-using System.Buffers;
-using System.Collections.Generic;
-using DotRecast.Core.Buffers;
-using DotRecast.Core.Collections;
-using NUnit.Framework;
-
-namespace DotRecast.Core.Test;
-
-public class RcArrayBenchmarkTests
-{
-    private const int StepLength = 512;
-    private const int RandomLoop = 1000;
-    private readonly RcRand _rand = new RcRand();
-
-    private (string title, long ticks) Bench(string title, Action<int> source)
-    {
-        var begin = RcFrequency.Ticks;
-        for (int step = StepLength; step > 0; --step)
-        {
-            for (int i = 0; i < RandomLoop; ++i)
-            {
-                source.Invoke(step);
-            }
-        }
-
-        var end = RcFrequency.Ticks - begin;
-        return (title, end);
-    }
-
-
-    private void RoundForArray(int len)
-    {
-        var array = new int[len];
-        for (int ii = 0; ii < len; ++ii)
-        {
-            array[ii] = _rand.NextInt32();
-        }
-    }
-
-
-    private void RoundForPureRentArray(int len)
-    {
-        var array = ArrayPool<int>.Shared.Rent(len);
-        for (int ii = 0; ii < array.Length; ++ii)
-        {
-            array[ii] = _rand.NextInt32();
-        }
-
-        ArrayPool<int>.Shared.Return(array);
-    }
-
-
-    private void RoundForRcRentedArray(int len)
-    {
-        using var rentedArray = RcRentedArray.Rent<int>(len);
-        var array = rentedArray.AsArray();
-        for (int i = 0; i < rentedArray.Length; ++i)
-        {
-            array[i] = _rand.NextInt32();
-        }
-    }
-
-
-    private void RoundForRcStackArray(int len)
-    {
-        var array = new RcStackArray512<int>();
-        for (int ii = 0; ii < len; ++ii)
-        {
-            array[ii] = _rand.NextInt32();
-        }
-    }
-
-    private void RoundForStackalloc(int len)
-    {
-        Span<int> array = stackalloc int[len];
-        for (int ii = 0; ii < len; ++ii)
-        {
-            array[ii] = _rand.NextInt32();
-        }
-    }
-
-
-    [Test]
-    public void TestBenchmarkArrays()
-    {
-        var list = new List<(string title, long ticks)>();
-        list.Add(Bench("new int[len]", RoundForArray));
-        list.Add(Bench("ArrayPool<int>.Shared.Rent(len)", RoundForPureRentArray));
-        list.Add(Bench("RcRentedArray.Rent<int>(len)", RoundForRcRentedArray));
-        list.Add(Bench("new RcStackArray512<int>()", RoundForRcStackArray));
-        list.Add(Bench("stackalloc int[len]", RoundForStackalloc));
-
-        list.Sort((x, y) => x.ticks.CompareTo(y.ticks));
-
-        foreach (var t in list)
-        {
-            Console.WriteLine($"{t.title} {t.ticks / (double)TimeSpan.TicksPerMillisecond} ms");
-        }
-    }
-}

test/DotRecast.Core.Test/RcRentedArrayTest.cs

@@ -31,7 +31,7 @@ public class RcRentedArrayTest
             {
                 int length = Math.Max(2, (int)(rand.Next() * 2048));
                 var values = RandomValues(length);
-                using var array = RcRentedArray.Rent<int>(length);
+                using var array = RcRentedArray.RentDisposableArray<int>(length);
 
                 for (int i = 0; i < array.Length; ++i)
                 {

test/DotRecast.Detour.Crowd.Test/DtPathCorridorTest.cs

@@ -0,0 +1,228 @@
+/*
+recast4j copyright (c) 2021 Piotr Piastucki piotr@jtilia.org
+DotRecast Copyright (c) 2023 Choi Ikpil ikpil@naver.com
+
+This software is provided 'as-is', without any express or implied
+warranty.  In no event will the authors be held liable for any damages
+arising from the use of this software.
+Permission is granted to anyone to use this software for any purpose,
+including commercial applications, and to alter it and redistribute it
+freely, subject to the following restrictions:
+1. The origin of this software must not be misrepresented; you must not
+ claim that you wrote the original software. If you use this software
+ in a product, an acknowledgment in the product documentation would be
+ appreciated but is not required.
+2. Altered source versions must be plainly marked as such, and must not be
+ misrepresented as being the original software.
+3. This notice may not be removed or altered from any source distribution.
+*/
+
+using System.Collections.Generic;
+using DotRecast.Core.Numerics;
+using Moq;
+using NUnit.Framework;
+
+namespace DotRecast.Detour.Crowd.Test;
+
+public class DtPathCorridorTest
+{
+    private DtPathCorridor corridor;
+    private IDtQueryFilter filter;
+
+    [SetUp]
+    public void SetUp()
+    {
+        corridor = new DtPathCorridor();
+        corridor.Init(DtCrowdConst.MAX_PATH_RESULT);
+        corridor.Reset(0, new RcVec3f(10, 20, 30));
+
+        filter = new DtQueryDefaultFilter();
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldHandleEmptyInput()
+    {
+        var path = new List<long>();
+        const int npath = 0;
+        const int maxPath = 0;
+        var visited = new List<long>();
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(0));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldHandleEmptyVisited()
+    {
+        var path = new List<long> { 1 };
+        const int npath = 1;
+        const int maxPath = 1;
+
+        var visited = new List<long>();
+
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(1));
+
+        var expectedPath = new List<long> { 1 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldHandleEmptyPath()
+    {
+        var path = new List<long>();
+        const int npath = 0;
+        const int maxPath = 0;
+
+        var visited = new List<long> { 1 };
+
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(0));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldStripVisitedPointsFromPathExceptLast()
+    {
+        var path = new List<long> { 1, 2 };
+        const int npath = 2;
+        const int maxPath = 2;
+
+        var visited = new List<long> { 1, 2 };
+
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(1));
+
+        var expectedPath = new List<long> { 2, 2 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldAddVisitedPointsNotPresentInPathInReverseOrder()
+    {
+        var path = new List<long> { 1, 2, 0 };
+        const int npath = 2;
+        const int maxPath = 3;
+        var visited = new List<long> { 1, 2, 3, 4 };
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(3));
+
+        var expectedPath = new List<long> { 4, 3, 2 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldAddVisitedPointsNotPresentInPathUpToThePathCapacity()
+    {
+        var path = new List<long>() { 1, 2, 0 };
+        const int npath = 2;
+        const int maxPath = 3;
+        var visited = new List<long>() { 1, 2, 3, 4, 5 };
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(3));
+
+        var expectedPath = new List<long> { 5, 4, 3 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldNotChangePathIfThereIsNoIntersectionWithVisited()
+    {
+        var path = new List<long>() { 1, 2 };
+        const int npath = 2;
+        const int maxPath = 2;
+        var visited = new List<long>() { 3, 4 };
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(2));
+
+        var expectedPath = new List<long> { 1, 2 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldSaveUnvisitedPathPoints()
+    {
+        var path = new List<long>() { 1, 2, 0 };
+        const int npath = 2;
+        const int maxPath = 3;
+        var visited = new List<long>() { 1, 3 };
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(3));
+
+        var expectedPath = new List<long> { 3, 1, 2 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test(Description = "dtMergeCorridorStartMoved")]
+    public void ShouldSaveUnvisitedPathPointsUpToThePathCapacity()
+    {
+        var path = new List<long>() { 1, 2 };
+        const int npath = 2;
+        const int maxPath = 2;
+        var visited = new List<long>() { 1, 3 };
+        var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited);
+        Assert.That(result.Count, Is.EqualTo(2));
+
+        var expectedPath = new List<long> { 3, 1 };
+        Assert.That(path, Is.EqualTo(expectedPath));
+    }
+
+    [Test]
+    public void ShouldKeepOriginalPathInFindCornersWhenNothingCanBePruned()
+    {
+        List<DtStraightPath> straightPath = new();
+        straightPath.Add(new DtStraightPath(new RcVec3f(11, 20, 30.00001f), 0, 0));
+        straightPath.Add(new DtStraightPath(new RcVec3f(12, 20, 30.00002f), 0, 0));
+        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
+        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
+        var mockQuery = new Mock<DtNavMeshQuery>(It.IsAny<DtNavMesh>());
+        mockQuery.Setup(q => q.FindStraightPath(
+                It.IsAny<RcVec3f>(),
+                It.IsAny<RcVec3f>(),
+                It.IsAny<List<long>>(),
+                ref It.Ref<List<DtStraightPath>>.IsAny,
+                It.IsAny<int>(),
+                It.IsAny<int>())
+            )
+            .Callback((RcVec3f startPos, RcVec3f endPos, List<long> path,
+                ref List<DtStraightPath> refStraightPath, int maxStraightPath, int options) =>
+            {
+                refStraightPath = straightPath;
+            })
+            .Returns(() => DtStatus.DT_SUCCESS);
+
+        var path = new List<DtStraightPath>();
+        corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter);
+        Assert.That(path.Count, Is.EqualTo(4));
+        Assert.That(path, Is.EqualTo(straightPath));
+    }
+
+    [Test]
+    public void ShouldPrunePathInFindCorners()
+    {
+        List<DtStraightPath> straightPath = new();
+        straightPath.Add(new DtStraightPath(new RcVec3f(10, 20, 30.00001f), 0, 0)); // too close
+        straightPath.Add(new DtStraightPath(new RcVec3f(10, 20, 30.00002f), 0, 0)); // too close
+        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
+        straightPath.Add(new DtStraightPath(new RcVec3f(12f, 22, 33f), DtStraightPathFlags.DT_STRAIGHTPATH_OFFMESH_CONNECTION, 0)); // offmesh
+        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), DtStraightPathFlags.DT_STRAIGHTPATH_OFFMESH_CONNECTION, 0)); // offmesh
+
+        var mockQuery = new Mock<DtNavMeshQuery>(It.IsAny<DtNavMesh>());
+        mockQuery.Setup(q => q.FindStraightPath(
+                It.IsAny<RcVec3f>(),
+                It.IsAny<RcVec3f>(),
+                It.IsAny<List<long>>(),
+                ref It.Ref<List<DtStraightPath>>.IsAny,
+                It.IsAny<int>(),
+                It.IsAny<int>())
+            ).Callback((RcVec3f startPos, RcVec3f endPos, List<long> path,
+                ref List<DtStraightPath> refStraightPath, int maxStraightPath, int options) =>
+            {
+                refStraightPath = straightPath;
+            })
+            .Returns(() => DtStatus.DT_SUCCESS);
+
+        var path = new List<DtStraightPath>();
+        corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter);
+        Assert.That(path.Count, Is.EqualTo(2));
+        Assert.That(path, Is.EqualTo(new List<DtStraightPath> { straightPath[2], straightPath[3] }));
+    }
+}

test/DotRecast.Detour.Crowd.Test/PathCorridorTest.cs

@@ -1,101 +0,0 @@
-/*
-recast4j copyright (c) 2021 Piotr Piastucki piotr@jtilia.org
-DotRecast Copyright (c) 2023 Choi Ikpil ikpil@naver.com
-
-This software is provided 'as-is', without any express or implied
-warranty.  In no event will the authors be held liable for any damages
-arising from the use of this software.
-Permission is granted to anyone to use this software for any purpose,
-including commercial applications, and to alter it and redistribute it
-freely, subject to the following restrictions:
-1. The origin of this software must not be misrepresented; you must not
- claim that you wrote the original software. If you use this software
- in a product, an acknowledgment in the product documentation would be
- appreciated but is not required.
-2. Altered source versions must be plainly marked as such, and must not be
- misrepresented as being the original software.
-3. This notice may not be removed or altered from any source distribution.
-*/
-
-using System.Collections.Generic;
-using DotRecast.Core.Numerics;
-
-using Moq;
-using NUnit.Framework;
-
-namespace DotRecast.Detour.Crowd.Test;
-
-
-public class PathCorridorTest
-{
-    private readonly DtPathCorridor corridor = new DtPathCorridor();
-    private readonly IDtQueryFilter filter = new DtQueryDefaultFilter();
-
-    [SetUp]
-    public void SetUp()
-    {
-        corridor.Init(256);
-        corridor.Reset(0, new RcVec3f(10, 20, 30));
-    }
-
-    [Test]
-    public void ShouldKeepOriginalPathInFindCornersWhenNothingCanBePruned()
-    {
-        List<DtStraightPath> straightPath = new();
-        straightPath.Add(new DtStraightPath(new RcVec3f(11, 20, 30.00001f), 0, 0));
-        straightPath.Add(new DtStraightPath(new RcVec3f(12, 20, 30.00002f), 0, 0));
-        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
-        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
-        var mockQuery = new Mock<DtNavMeshQuery>(It.IsAny<DtNavMesh>());
-        mockQuery.Setup(q => q.FindStraightPath(
-                It.IsAny<RcVec3f>(),
-                It.IsAny<RcVec3f>(),
-                It.IsAny<List<long>>(),
-                ref It.Ref<List<DtStraightPath>>.IsAny,
-                It.IsAny<int>(),
-                It.IsAny<int>())
-            )
-            .Callback((RcVec3f startPos, RcVec3f endPos, List<long> path,
-                ref List<DtStraightPath> refStraightPath, int maxStraightPath, int options) =>
-            {
-                refStraightPath = straightPath;
-            })
-            .Returns(() => DtStatus.DT_SUCCESS);
-
-        var path = new List<DtStraightPath>();
-        corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter);
-        Assert.That(path.Count, Is.EqualTo(4));
-        Assert.That(path, Is.EqualTo(straightPath));
-    }
-
-    [Test]
-    public void ShouldPrunePathInFindCorners()
-    {
-        List<DtStraightPath> straightPath = new();
-        straightPath.Add(new DtStraightPath(new RcVec3f(10, 20, 30.00001f), 0, 0)); // too close
-        straightPath.Add(new DtStraightPath(new RcVec3f(10, 20, 30.00002f), 0, 0)); // too close
-        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), 0, 0));
-        straightPath.Add(new DtStraightPath(new RcVec3f(12f, 22, 33f), DtStraightPathFlags.DT_STRAIGHTPATH_OFFMESH_CONNECTION, 0)); // offmesh
-        straightPath.Add(new DtStraightPath(new RcVec3f(11f, 21, 32f), DtStraightPathFlags.DT_STRAIGHTPATH_OFFMESH_CONNECTION, 0)); // offmesh
-
-        var mockQuery = new Mock<DtNavMeshQuery>(It.IsAny<DtNavMesh>());
-        mockQuery.Setup(q => q.FindStraightPath(
-                It.IsAny<RcVec3f>(),
-                It.IsAny<RcVec3f>(),
-                It.IsAny<List<long>>(),
-                ref It.Ref<List<DtStraightPath>>.IsAny,
-                It.IsAny<int>(),
-                It.IsAny<int>())
-            ).Callback((RcVec3f startPos, RcVec3f endPos, List<long> path,
-                ref List<DtStraightPath> refStraightPath, int maxStraightPath, int options) =>
-            {
-                refStraightPath = straightPath;
-            })
-            .Returns(() => DtStatus.DT_SUCCESS);
-
-        var path = new List<DtStraightPath>();
-        corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter);
-        Assert.That(path.Count, Is.EqualTo(2));
-        Assert.That(path, Is.EqualTo(new List<DtStraightPath> { straightPath[2], straightPath[3] }));
-    }
-}