From eb1aa4936368deb33c60cf657d96201c192f0a65 Mon Sep 17 00:00:00 2001 From: ikpil Date: Sun, 11 Feb 2024 17:15:22 +0900 Subject: [PATCH] [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. - https://github.com/recastnavigation/recastnavigation/commit/599fd0f023181c0a484df2a18cf1d75a3553852e - https://github.com/recastnavigation/recastnavigation/pull/635 replace comment in DtPathCorridor checking tt npath --- src/DotRecast.Detour/DtPathUtils.cs | 1 + .../DtPathCorridorTest.cs | 228 ++++++++++++++++++ .../PathCorridorTest.cs | 101 -------- 3 files changed, 229 insertions(+), 101 deletions(-) create mode 100644 test/DotRecast.Detour.Crowd.Test/DtPathCorridorTest.cs delete mode 100644 test/DotRecast.Detour.Crowd.Test/PathCorridorTest.cs diff --git a/src/DotRecast.Detour/DtPathUtils.cs b/src/DotRecast.Detour/DtPathUtils.cs index 7534f29..b9c77e1 100644 --- a/src/DotRecast.Detour/DtPathUtils.cs +++ b/src/DotRecast.Detour/DtPathUtils.cs @@ -176,6 +176,7 @@ namespace DotRecast.Detour // Adjust beginning of the buffer to include the visited. List result = new List(); + // Store visited for (int i = visited.Count - 1; i > furthestVisited; --i) { diff --git a/test/DotRecast.Detour.Crowd.Test/DtPathCorridorTest.cs b/test/DotRecast.Detour.Crowd.Test/DtPathCorridorTest.cs new file mode 100644 index 0000000..740188e --- /dev/null +++ b/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(); + const int npath = 0; + const int maxPath = 0; + var visited = new List(); + 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 { 1 }; + const int npath = 1; + const int maxPath = 1; + + var visited = new List(); + + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(1)); + + var expectedPath = new List { 1 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldHandleEmptyPath() + { + var path = new List(); + const int npath = 0; + const int maxPath = 0; + + var visited = new List { 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 { 1, 2 }; + const int npath = 2; + const int maxPath = 2; + + var visited = new List { 1, 2 }; + + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(1)); + + var expectedPath = new List { 2, 2 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldAddVisitedPointsNotPresentInPathInReverseOrder() + { + var path = new List { 1, 2, 0 }; + const int npath = 2; + const int maxPath = 3; + var visited = new List { 1, 2, 3, 4 }; + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(3)); + + var expectedPath = new List { 4, 3, 2 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldAddVisitedPointsNotPresentInPathUpToThePathCapacity() + { + var path = new List() { 1, 2, 0 }; + const int npath = 2; + const int maxPath = 3; + var visited = new List() { 1, 2, 3, 4, 5 }; + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(3)); + + var expectedPath = new List { 5, 4, 3 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldNotChangePathIfThereIsNoIntersectionWithVisited() + { + var path = new List() { 1, 2 }; + const int npath = 2; + const int maxPath = 2; + var visited = new List() { 3, 4 }; + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(2)); + + var expectedPath = new List { 1, 2 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldSaveUnvisitedPathPoints() + { + var path = new List() { 1, 2, 0 }; + const int npath = 2; + const int maxPath = 3; + var visited = new List() { 1, 3 }; + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(3)); + + var expectedPath = new List { 3, 1, 2 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test(Description = "dtMergeCorridorStartMoved")] + public void ShouldSaveUnvisitedPathPointsUpToThePathCapacity() + { + var path = new List() { 1, 2 }; + const int npath = 2; + const int maxPath = 2; + var visited = new List() { 1, 3 }; + var result = DtPathUtils.MergeCorridorStartMoved(path, npath, maxPath, visited); + Assert.That(result.Count, Is.EqualTo(2)); + + var expectedPath = new List { 3, 1 }; + Assert.That(path, Is.EqualTo(expectedPath)); + } + + [Test] + public void ShouldKeepOriginalPathInFindCornersWhenNothingCanBePruned() + { + List 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(It.IsAny()); + mockQuery.Setup(q => q.FindStraightPath( + It.IsAny(), + It.IsAny(), + It.IsAny>(), + ref It.Ref>.IsAny, + It.IsAny(), + It.IsAny()) + ) + .Callback((RcVec3f startPos, RcVec3f endPos, List path, + ref List refStraightPath, int maxStraightPath, int options) => + { + refStraightPath = straightPath; + }) + .Returns(() => DtStatus.DT_SUCCESS); + + var path = new List(); + 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 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(It.IsAny()); + mockQuery.Setup(q => q.FindStraightPath( + It.IsAny(), + It.IsAny(), + It.IsAny>(), + ref It.Ref>.IsAny, + It.IsAny(), + It.IsAny()) + ).Callback((RcVec3f startPos, RcVec3f endPos, List path, + ref List refStraightPath, int maxStraightPath, int options) => + { + refStraightPath = straightPath; + }) + .Returns(() => DtStatus.DT_SUCCESS); + + var path = new List(); + corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter); + Assert.That(path.Count, Is.EqualTo(2)); + Assert.That(path, Is.EqualTo(new List { straightPath[2], straightPath[3] })); + } +} \ No newline at end of file diff --git a/test/DotRecast.Detour.Crowd.Test/PathCorridorTest.cs b/test/DotRecast.Detour.Crowd.Test/PathCorridorTest.cs deleted file mode 100644 index fdc10c1..0000000 --- a/test/DotRecast.Detour.Crowd.Test/PathCorridorTest.cs +++ /dev/null @@ -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 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(It.IsAny()); - mockQuery.Setup(q => q.FindStraightPath( - It.IsAny(), - It.IsAny(), - It.IsAny>(), - ref It.Ref>.IsAny, - It.IsAny(), - It.IsAny()) - ) - .Callback((RcVec3f startPos, RcVec3f endPos, List path, - ref List refStraightPath, int maxStraightPath, int options) => - { - refStraightPath = straightPath; - }) - .Returns(() => DtStatus.DT_SUCCESS); - - var path = new List(); - 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 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(It.IsAny()); - mockQuery.Setup(q => q.FindStraightPath( - It.IsAny(), - It.IsAny(), - It.IsAny>(), - ref It.Ref>.IsAny, - It.IsAny(), - It.IsAny()) - ).Callback((RcVec3f startPos, RcVec3f endPos, List path, - ref List refStraightPath, int maxStraightPath, int options) => - { - refStraightPath = straightPath; - }) - .Returns(() => DtStatus.DT_SUCCESS); - - var path = new List(); - corridor.FindCorners(ref path, int.MaxValue, mockQuery.Object, filter); - Assert.That(path.Count, Is.EqualTo(2)); - Assert.That(path, Is.EqualTo(new List { straightPath[2], straightPath[3] })); - } -} \ No newline at end of file