Check input and report errors via RecastContext

Recast functions have preconditions for arguments they don't validate. This may
produce garbage data which may lead to crash. Check arguments and log when they
are invalid.

Do not throw exceptions when these function calls fail, capture Recast reported
errors via RecastContext inherited from rcContext and log them.

apps/openmw_test_suite/detournavigator/navigator.cpp

@@ -1203,4 +1203,29 @@ namespace
 
         EXPECT_EQ(mNavigator->getNavMesh(mAgentBounds)->lockConst()->getVersion(), version);
     }
+
+    TEST_F(DetourNavigatorNavigatorTest, add_agent_with_zero_coordinate_should_not_have_nav_mesh)
+    {
+        constexpr std::array<float, 5 * 5> heightfieldData{ {
+            0, 0, 0, 0, 0, // row 0
+            0, -25, -25, -25, -25, // row 1
+            0, -25, -100, -100, -100, // row 2
+            0, -25, -100, -100, -100, // row 3
+            0, -25, -100, -100, -100, // row 4
+        } };
+        const HeightfieldSurface surface = makeSquareHeightfieldSurface(heightfieldData);
+        const int cellSize = mHeightfieldTileSize * (surface.mSize - 1);
+
+        const AgentBounds agentBounds{ CollisionShapeType::RotatingBox, { 0, 1, 1 } };
+        mNavigator->addAgent(agentBounds);
+        auto updateGuard = mNavigator->makeUpdateGuard();
+        mNavigator->addHeightfield(mCellPosition, cellSize, surface, updateGuard.get());
+        mNavigator->update(mPlayerPosition, updateGuard.get());
+        updateGuard.reset();
+        mNavigator->wait(WaitConditionType::requiredTilesPresent, &mListener);
+
+        EXPECT_EQ(
+            findPath(*mNavigator, agentBounds, mStepSize, mStart, mEnd, Flag_walk, mAreaCosts, mEndTolerance, mOut),
+            Status::StartPolygonNotFound);
+    }
 }

components/CMakeLists.txt

@@ -315,6 +315,7 @@ add_component_dir(detournavigator
     collisionshapetype
     stats
     commulativeaabb
+    recastcontext
     )
 
 add_component_dir(loadinglistener

components/detournavigator/debug.cpp

@@ -86,7 +86,8 @@ namespace DetourNavigator
 
     std::ostream& operator<<(std::ostream& s, const AgentBounds& v)
     {
-        return s << "AgentBounds {" << v.mShapeType << ", " << v.mHalfExtents << "}";
+        return s << "AgentBounds {" << v.mShapeType << ", {" << v.mHalfExtents.x() << ", " << v.mHalfExtents.y() << ", "
+                 << v.mHalfExtents.z() << "}}";
     }
 
     namespace

components/detournavigator/makenavmesh.cpp

@@ -7,6 +7,7 @@
 #include "navmeshtilescache.hpp"
 #include "offmeshconnection.hpp"
 #include "preparednavmeshdata.hpp"
+#include "recastcontext.hpp"
 #include "recastmesh.hpp"
 #include "recastmeshbuilder.hpp"
 #include "recastparams.hpp"
@@ -14,6 +15,8 @@
 #include "settingsutils.hpp"
 #include "sharednavmesh.hpp"
 
+#include "components/debug/debuglog.hpp"
+
 #include <DetourNavMesh.h>
 #include <DetourNavMeshBuilder.h>
 #include <Recast.h>
@@ -138,8 +141,8 @@ namespace DetourNavigator
             return result;
         }
 
-        void initHeightfield(rcContext& context, const TilePosition& tilePosition, float minZ, float maxZ,
+        [[nodiscard]] bool initHeightfield(RecastContext& context, const TilePosition& tilePosition, float minZ,
-            const RecastSettings& settings, rcHeightfield& solid)
+            float maxZ, const RecastSettings& settings, rcHeightfield& solid)
         {
             const int size = settings.mTileSize + settings.mBorderSize * 2;
             const int width = size;
@@ -150,14 +153,37 @@ namespace DetourNavigator
             const osg::Vec3f bmin(shift.x() - halfBoundsSize, minZ, shift.y() - halfBoundsSize);
             const osg::Vec3f bmax(shift.x() + halfBoundsSize, maxZ, shift.y() + halfBoundsSize);
 
-            const auto result = rcCreateHeightfield(
+            if (width < 0)
-                &context, solid, width, height, bmin.ptr(), bmax.ptr(), settings.mCellSize, settings.mCellHeight);
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid width to init heightfield: " << width;
+                return false;
+            }
+
+            if (height < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid height to init heightfield: " << height;
+                return false;
+            }
+
+            if (settings.mCellHeight <= 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid cell height to init heightfield: " << settings.mCellHeight;
+                return false;
+            }
+
+            if (settings.mCellSize <= 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid cell size to init heightfield: " << settings.mCellSize;
+                return false;
+            }
 
-            if (!result)
+            return rcCreateHeightfield(
-                throw NavigatorException("Failed to create heightfield for navmesh");
+                &context, solid, width, height, bmin.ptr(), bmax.ptr(), settings.mCellSize, settings.mCellHeight);
         }
 
-        bool rasterizeTriangles(rcContext& context, const Mesh& mesh, const RecastSettings& settings,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const Mesh& mesh, const RecastSettings& settings,
             const RecastParams& params, rcHeightfield& solid)
         {
             std::vector<unsigned char> areas(mesh.getAreaTypes().begin(), mesh.getAreaTypes().end());
@@ -178,7 +204,7 @@ namespace DetourNavigator
                 mesh.getIndices().data(), areas.data(), static_cast<int>(areas.size()), solid, params.mWalkableClimb);
         }
 
-        bool rasterizeTriangles(rcContext& context, const Rectangle& rectangle, AreaType areaType,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const Rectangle& rectangle, AreaType areaType,
             const RecastParams& params, rcHeightfield& solid)
         {
             const std::array vertices{
@@ -199,9 +225,9 @@ namespace DetourNavigator
                 indices.data(), areas.data(), static_cast<int>(areas.size()), solid, params.mWalkableClimb);
         }
 
-        bool rasterizeTriangles(rcContext& context, float agentHalfExtentsZ, const std::vector<CellWater>& water,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, float agentHalfExtentsZ,
-            const RecastSettings& settings, const RecastParams& params, const TileBounds& realTileBounds,
+            const std::vector<CellWater>& water, const RecastSettings& settings, const RecastParams& params,
-            rcHeightfield& solid)
+            const TileBounds& realTileBounds, rcHeightfield& solid)
         {
             for (const CellWater& cellWater : water)
             {
@@ -219,7 +245,7 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const TileBounds& realTileBounds,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const TileBounds& realTileBounds,
             const std::vector<FlatHeightfield>& heightfields, const RecastSettings& settings,
             const RecastParams& params, rcHeightfield& solid)
         {
@@ -237,7 +263,7 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const std::vector<Heightfield>& heightfields,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const std::vector<Heightfield>& heightfields,
             const RecastSettings& settings, const RecastParams& params, rcHeightfield& solid)
         {
             for (const Heightfield& heightfield : heightfields)
@@ -249,9 +275,9 @@ namespace DetourNavigator
             return true;
         }
 
-        bool rasterizeTriangles(rcContext& context, const TilePosition& tilePosition, float agentHalfExtentsZ,
+        [[nodiscard]] bool rasterizeTriangles(RecastContext& context, const TilePosition& tilePosition,
-            const RecastMesh& recastMesh, const RecastSettings& settings, const RecastParams& params,
+            float agentHalfExtentsZ, const RecastMesh& recastMesh, const RecastSettings& settings,
-            rcHeightfield& solid)
+            const RecastParams& params, rcHeightfield& solid)
         {
             const TileBounds realTileBounds = makeRealTileBoundsWithBorder(settings, tilePosition);
             return rasterizeTriangles(context, recastMesh.getMesh(), settings, params, solid)
@@ -262,66 +288,119 @@ namespace DetourNavigator
                     context, realTileBounds, recastMesh.getFlatHeightfields(), settings, params, solid);
         }
 
-        void buildCompactHeightfield(rcContext& context, const int walkableHeight, const int walkableClimb,
+        [[nodiscard]] bool buildCompactHeightfield(RecastContext& context, const int walkableHeight,
-            rcHeightfield& solid, rcCompactHeightfield& compact)
+            const int walkableClimb, rcHeightfield& solid, rcCompactHeightfield& compact)
         {
-            const auto result = rcBuildCompactHeightfield(&context, walkableHeight, walkableClimb, solid, compact);
+            if (walkableHeight < 3)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableHeight to build compact heightfield: " << walkableHeight;
+                return false;
+            }
+
+            if (walkableClimb < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableClimb to build compact heightfield: " << walkableClimb;
+                return false;
+            }
 
-            if (!result)
+            return rcBuildCompactHeightfield(&context, walkableHeight, walkableClimb, solid, compact);
-                throw NavigatorException("Failed to build compact heightfield for navmesh");
         }
 
-        void erodeWalkableArea(rcContext& context, int walkableRadius, rcCompactHeightfield& compact)
+        [[nodiscard]] bool erodeWalkableArea(RecastContext& context, int walkableRadius, rcCompactHeightfield& compact)
         {
-            const auto result = rcErodeWalkableArea(&context, walkableRadius, compact);
+            if (walkableRadius <= 0 || 255 <= walkableRadius)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid walkableRadius to erode walkable area: " << walkableRadius;
+                return false;
+            }
 
-            if (!result)
+            return rcErodeWalkableArea(&context, walkableRadius, compact);
-                throw NavigatorException("Failed to erode walkable area for navmesh");
         }
 
-        void buildDistanceField(rcContext& context, rcCompactHeightfield& compact)
+        [[nodiscard]] bool buildDistanceField(RecastContext& context, rcCompactHeightfield& compact)
         {
-            const auto result = rcBuildDistanceField(&context, compact);
+            return rcBuildDistanceField(&context, compact);
-
-            if (!result)
-                throw NavigatorException("Failed to build distance field for navmesh");
         }
 
-        void buildRegions(rcContext& context, rcCompactHeightfield& compact, const int borderSize,
+        [[nodiscard]] bool buildRegions(RecastContext& context, rcCompactHeightfield& compact, const int borderSize,
             const int minRegionArea, const int mergeRegionArea)
         {
-            const auto result = rcBuildRegions(&context, compact, borderSize, minRegionArea, mergeRegionArea);
+            if (borderSize < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid borderSize to build regions: " << borderSize;
+                return false;
+            }
+
+            if (minRegionArea < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid minRegionArea to build regions: " << minRegionArea;
+                return false;
+            }
 
-            if (!result)
+            if (mergeRegionArea < 0)
-                throw NavigatorException("Failed to build distance field for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid mergeRegionArea to build regions: " << mergeRegionArea;
+                return false;
+            }
+
+            return rcBuildRegions(&context, compact, borderSize, minRegionArea, mergeRegionArea);
         }
 
-        void buildContours(rcContext& context, rcCompactHeightfield& compact, const float maxError,
+        [[nodiscard]] bool buildContours(RecastContext& context, rcCompactHeightfield& compact, const float maxError,
             const int maxEdgeLen, rcContourSet& contourSet, const int buildFlags = RC_CONTOUR_TESS_WALL_EDGES)
         {
-            const auto result = rcBuildContours(&context, compact, maxError, maxEdgeLen, contourSet, buildFlags);
+            if (maxError < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid maxError to build contours: " << maxError;
+                return false;
+            }
+
+            if (maxEdgeLen < 0)
+            {
+                Log(Debug::Warning) << context.getPrefix() << "Invalid maxEdgeLen to build contours: " << maxEdgeLen;
+                return false;
+            }
 
-            if (!result)
+            return rcBuildContours(&context, compact, maxError, maxEdgeLen, contourSet, buildFlags);
-                throw NavigatorException("Failed to build contours for navmesh");
         }
 
-        void buildPolyMesh(
+        [[nodiscard]] bool buildPolyMesh(
-            rcContext& context, rcContourSet& contourSet, const int maxVertsPerPoly, rcPolyMesh& polyMesh)
+            RecastContext& context, rcContourSet& contourSet, const int maxVertsPerPoly, rcPolyMesh& polyMesh)
         {
-            const auto result = rcBuildPolyMesh(&context, contourSet, maxVertsPerPoly, polyMesh);
+            if (maxVertsPerPoly < 3)
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid maxVertsPerPoly to build poly mesh: " << maxVertsPerPoly;
+                return false;
+            }
 
-            if (!result)
+            return rcBuildPolyMesh(&context, contourSet, maxVertsPerPoly, polyMesh);
-                throw NavigatorException("Failed to build poly mesh for navmesh");
         }
 
-        void buildPolyMeshDetail(rcContext& context, const rcPolyMesh& polyMesh, const rcCompactHeightfield& compact,
+        [[nodiscard]] bool buildPolyMeshDetail(RecastContext& context, const rcPolyMesh& polyMesh,
-            const float sampleDist, const float sampleMaxError, rcPolyMeshDetail& polyMeshDetail)
+            const rcCompactHeightfield& compact, const float sampleDist, const float sampleMaxError,
+            rcPolyMeshDetail& polyMeshDetail)
         {
-            const auto result
+            if (sampleDist < 0)
-                = rcBuildPolyMeshDetail(&context, polyMesh, compact, sampleDist, sampleMaxError, polyMeshDetail);
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid sampleDist to build poly mesh detail: " << sampleDist;
+                return false;
+            }
 
-            if (!result)
+            if (sampleMaxError < 0)
-                throw NavigatorException("Failed to build detail poly mesh for navmesh");
+            {
+                Log(Debug::Warning) << context.getPrefix()
+                                    << "Invalid sampleMaxError to build poly mesh detail: " << sampleMaxError;
+                return false;
+            }
+
+            return rcBuildPolyMeshDetail(&context, polyMesh, compact, sampleDist, sampleMaxError, polyMeshDetail);
         }
 
         void setPolyMeshFlags(rcPolyMesh& polyMesh)
@@ -330,25 +409,36 @@ namespace DetourNavigator
                 polyMesh.flags[i] = getFlag(static_cast<AreaType>(polyMesh.areas[i]));
         }
 
-        bool fillPolyMesh(rcContext& context, const RecastSettings& settings, const RecastParams& params,
+        [[nodiscard]] bool fillPolyMesh(RecastContext& context, const RecastSettings& settings,
-            rcHeightfield& solid, rcPolyMesh& polyMesh, rcPolyMeshDetail& polyMeshDetail)
+            const RecastParams& params, rcHeightfield& solid, rcPolyMesh& polyMesh, rcPolyMeshDetail& polyMeshDetail)
         {
             rcCompactHeightfield compact;
-            buildCompactHeightfield(context, params.mWalkableHeight, params.mWalkableClimb, solid, compact);
+            if (!buildCompactHeightfield(context, params.mWalkableHeight, params.mWalkableClimb, solid, compact))
+                return false;
 
-            erodeWalkableArea(context, params.mWalkableRadius, compact);
+            if (!erodeWalkableArea(context, params.mWalkableRadius, compact))
-            buildDistanceField(context, compact);
+                return false;
-            buildRegions(context, compact, settings.mBorderSize, settings.mRegionMinArea, settings.mRegionMergeArea);
+
+            if (!buildDistanceField(context, compact))
+                return false;
+
+            if (!buildRegions(
+                    context, compact, settings.mBorderSize, settings.mRegionMinArea, settings.mRegionMergeArea))
+                return false;
 
             rcContourSet contourSet;
-            buildContours(context, compact, settings.mMaxSimplificationError, params.mMaxEdgeLen, contourSet);
+            if (!buildContours(context, compact, settings.mMaxSimplificationError, params.mMaxEdgeLen, contourSet))
+                return false;
 
             if (contourSet.nconts == 0)
                 return false;
 
-            buildPolyMesh(context, contourSet, settings.mMaxVertsPerPoly, polyMesh);
+            if (!buildPolyMesh(context, contourSet, settings.mMaxVertsPerPoly, polyMesh))
+                return false;
 
-            buildPolyMeshDetail(context, polyMesh, compact, params.mSampleDist, params.mSampleMaxError, polyMeshDetail);
+            if (!buildPolyMeshDetail(
+                    context, polyMesh, compact, params.mSampleDist, params.mSampleMaxError, polyMeshDetail))
+                return false;
 
             setPolyMeshFlags(polyMesh);
 
@@ -410,13 +500,14 @@ namespace DetourNavigator
     std::unique_ptr<PreparedNavMeshData> prepareNavMeshTileData(const RecastMesh& recastMesh,
         const TilePosition& tilePosition, const AgentBounds& agentBounds, const RecastSettings& settings)
     {
-        rcContext context;
+        RecastContext context(tilePosition, agentBounds);
 
         const auto [minZ, maxZ] = getBoundsByZ(recastMesh, agentBounds.mHalfExtents.z(), settings);
 
         rcHeightfield solid;
-        initHeightfield(context, tilePosition, toNavMeshCoordinates(settings, minZ),
+        if (!initHeightfield(context, tilePosition, toNavMeshCoordinates(settings, minZ),
-            toNavMeshCoordinates(settings, maxZ), settings, solid);
+                toNavMeshCoordinates(settings, maxZ), settings, solid))
+            return nullptr;
 
         const RecastParams params = makeRecastParams(settings, agentBounds);
 

components/detournavigator/recastcontext.cpp

@@ -0,0 +1,45 @@
+#include "recastcontext.hpp"
+#include "debug.hpp"
+
+#include "components/debug/debuglog.hpp"
+
+#include <sstream>
+
+namespace DetourNavigator
+{
+    namespace
+    {
+        Debug::Level getLogLevel(rcLogCategory category)
+        {
+            switch (category)
+            {
+                case RC_LOG_PROGRESS:
+                    return Debug::Verbose;
+                case RC_LOG_WARNING:
+                    return Debug::Warning;
+                case RC_LOG_ERROR:
+                    return Debug::Error;
+            }
+            return Debug::Debug;
+        }
+
+        std::string formatPrefix(const TilePosition& tilePosition, const AgentBounds& agentBounds)
+        {
+            std::ostringstream stream;
+            stream << "Tile position: " << tilePosition.x() << ", " << tilePosition.y()
+                   << "; agent bounds: " << agentBounds << "; ";
+            return stream.str();
+        }
+    }
+
+    RecastContext::RecastContext(const TilePosition& tilePosition, const AgentBounds& agentBounds)
+        : mPrefix(formatPrefix(tilePosition, agentBounds))
+    {
+    }
+
+    void RecastContext::doLog(const rcLogCategory category, const char* msg, const int len)
+    {
+        if (len > 0)
+            Log(getLogLevel(category)) << mPrefix << std::string_view(msg, static_cast<std::size_t>(len));
+    }
+}

components/detournavigator/recastcontext.hpp

@@ -0,0 +1,28 @@
+#ifndef OPENMW_COMPONENTS_DETOURNAVIGATOR_RECASTCONTEXT_H
+#define OPENMW_COMPONENTS_DETOURNAVIGATOR_RECASTCONTEXT_H
+
+#include "tileposition.hpp"
+
+#include <string>
+
+#include <Recast.h>
+
+namespace DetourNavigator
+{
+    struct AgentBounds;
+
+    class RecastContext final : public rcContext
+    {
+    public:
+        explicit RecastContext(const TilePosition& tilePosition, const AgentBounds& agentBounds);
+
+        const std::string& getPrefix() const { return mPrefix; }
+
+    private:
+        std::string mPrefix;
+
+        void doLog(rcLogCategory category, const char* msg, int len) override;
+    };
+}
+
+#endif