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 recastnavigation#178

Detour/Include/DetourNavMeshQuery.h

@@ -172,7 +172,7 @@ class dtNavMeshQuery
 
 	/// 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);
 

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
 {

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)

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);