From 09afa02f4a51c4d55df58b8640b46026d195af8d Mon Sep 17 00:00:00 2001
From: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Date: Mon, 15 Feb 2016 22:54:53 +0100
Subject: [PATCH] Fail when too many nodes are requested

dtNavMeshQuery now fails initialization if too many nodes are requested
in the node pool. This could cause wrong paths and infinite loops to
happen if the node indices started overflowing dtNodeIndex or
dtNode::pidx.

Fix #178
---
 Detour/Include/DetourNavMeshQuery.h  |  2 +-
 Detour/Include/DetourNode.h          | 18 ++++++++++--------
 Detour/Source/DetourNavMeshQuery.cpp |  4 +++-
 Detour/Source/DetourNode.cpp         |  4 +++-
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/Detour/Include/DetourNavMeshQuery.h b/Detour/Include/DetourNavMeshQuery.h
index 6af61bf..5326600 100644
--- a/Detour/Include/DetourNavMeshQuery.h
+++ b/Detour/Include/DetourNavMeshQuery.h
@@ -172,7 +172,7 @@ public:
 	
 	/// Initializes the query object.
 	///  @param[in]		nav			Pointer to the dtNavMesh object to use for all queries.
-	///  @param[in]		maxNodes	Maximum number of search nodes. [Limits: 0 < value <= 65536]
+	///  @param[in]		maxNodes	Maximum number of search nodes. [Limits: 0 < value <= 65535]
 	/// @returns The status flags for the query.
 	dtStatus init(const dtNavMesh* nav, const int maxNodes);
 	
diff --git a/Detour/Include/DetourNode.h b/Detour/Include/DetourNode.h
index 1494e1e..db09747 100644
--- a/Detour/Include/DetourNode.h
+++ b/Detour/Include/DetourNode.h
@@ -31,18 +31,20 @@ enum dtNodeFlags
 typedef unsigned short dtNodeIndex;
 static const dtNodeIndex DT_NULL_IDX = (dtNodeIndex)~0;
 
+static const int DT_NODE_PARENT_BITS = 24;
+static const int DT_NODE_STATE_BITS = 2;
 struct dtNode
 {
-	float pos[3];				///< Position of the node.
-	float cost;					///< Cost from previous node to current node.
-	float total;				///< Cost up to the node.
-	unsigned int pidx : 24;		///< Index to parent node.
-	unsigned int state : 2;		///< extra state information. A polyRef can have multiple nodes with different extra info. see DT_MAX_STATES_PER_NODE
-	unsigned int flags : 3;		///< Node flags. A combination of dtNodeFlags.
-	dtPolyRef id;				///< Polygon ref the node corresponds to.
+	float pos[3];								///< Position of the node.
+	float cost;									///< Cost from previous node to current node.
+	float total;								///< Cost up to the node.
+	unsigned int pidx : DT_NODE_PARENT_BITS;	///< Index to parent node.
+	unsigned int state : DT_NODE_STATE_BITS;	///< extra state information. A polyRef can have multiple nodes with different extra info. see DT_MAX_STATES_PER_NODE
+	unsigned int flags : 3;						///< Node flags. A combination of dtNodeFlags.
+	dtPolyRef id;								///< Polygon ref the node corresponds to.
 };
 
-static const int DT_MAX_STATES_PER_NODE = 4;	// number of extra states per node. See dtNode::state
+static const int DT_MAX_STATES_PER_NODE = 1 << DT_NODE_STATE_BITS;	// number of extra states per node. See dtNode::state
 
 class dtNodePool
 {
diff --git a/Detour/Source/DetourNavMeshQuery.cpp b/Detour/Source/DetourNavMeshQuery.cpp
index 9db64d0..443a07c 100644
--- a/Detour/Source/DetourNavMeshQuery.cpp
+++ b/Detour/Source/DetourNavMeshQuery.cpp
@@ -165,6 +165,9 @@ dtNavMeshQuery::~dtNavMeshQuery()
 /// This function can be used multiple times.
 dtStatus dtNavMeshQuery::init(const dtNavMesh* nav, const int maxNodes)
 {
+	if (maxNodes > DT_NULL_IDX || maxNodes > (1 << DT_NODE_PARENT_BITS) - 1)
+		return DT_FAILURE | DT_INVALID_PARAM;
+
 	m_nav = nav;
 	
 	if (!m_nodePool || m_nodePool->getMaxNodes() < maxNodes)
@@ -195,7 +198,6 @@ dtStatus dtNavMeshQuery::init(const dtNavMesh* nav, const int maxNodes)
 		m_tinyNodePool->clear();
 	}
 	
-	// TODO: check the open list size too.
 	if (!m_openList || m_openList->getCapacity() < maxNodes)
 	{
 		if (m_openList)
diff --git a/Detour/Source/DetourNode.cpp b/Detour/Source/DetourNode.cpp
index 5cf6548..48abbba 100644
--- a/Detour/Source/DetourNode.cpp
+++ b/Detour/Source/DetourNode.cpp
@@ -57,7 +57,9 @@ dtNodePool::dtNodePool(int maxNodes, int hashSize) :
 	m_nodeCount(0)
 {
 	dtAssert(dtNextPow2(m_hashSize) == (unsigned int)m_hashSize);
-	dtAssert(m_maxNodes > 0);
+	// pidx is special as 0 means "none" and 1 is the first node. For that reason
+	// we have 1 fewer nodes available than the number of values it can contain.
+	dtAssert(m_maxNodes > 0 && m_maxNodes <= DT_NULL_IDX && m_maxNodes <= (1 << DT_NODE_PARENT_BITS) - 1);
 
 	m_nodes = (dtNode*)dtAlloc(sizeof(dtNode)*m_maxNodes, DT_ALLOC_PERM);
 	m_next = (dtNodeIndex*)dtAlloc(sizeof(dtNodeIndex)*m_maxNodes, DT_ALLOC_PERM);