diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index ab37028..0000000 --- a/.travis.yml +++ /dev/null @@ -1,11 +0,0 @@ -language: cpp -compiler: - - g++ - - clang - -script: - cd cpp - make - make test - - diff --git a/CMakeLists.txt b/CMakeLists.txt index b94df4e..7856c7e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,6 +8,7 @@ include(FetchContent) FetchContent_Declare( doctest URL https://raw.githubusercontent.com/doctest/doctest/v2.4.11/doctest/doctest.h + URL_HASH SHA256=44faa038e9c3f9728efbda143748d01124ea0a27f4bf78f35a15d8fab2e039fb DOWNLOAD_NO_EXTRACT TRUE ) FetchContent_MakeAvailable(doctest) diff --git a/GEMINI.md b/GEMINI.md index 022a95b..652af24 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -10,14 +10,15 @@ The core logic resides in `stlastar.h`, which uses C++ templates to work with an ### Prerequisites * C++ compiler supporting C++11 (e.g., `g++`, `clang++`). -* `make` utility. +* CMake 3.20 or newer. ### Build Commands -The project uses a `makefile` to manage builds. +The project uses CMake to configure and build. * **Build All:** ```bash - make + cmake -S . -B build -DCMAKE_BUILD_TYPE=Release + cmake --build build ``` This compiles the library examples and tests, producing the following executables: * `8puzzle`: Solves the 8-puzzle sliding tile game. @@ -28,12 +29,14 @@ The project uses a `makefile` to manage builds. * **Run Tests:** ```bash - make test + ctest --test-dir build --output-on-failure + # or run directly: ./build/tests ``` * **Clean Build:** ```bash - make clean + cmake --build build --target clean + # or: rm -rf build ``` ### Running Examples @@ -89,5 +92,5 @@ public: 6. Call `astarsearch.FreeSolutionNodes()` and `astarsearch.EnsureMemoryFreed()` to clean up. ### Testing -* Tests are located in `tests.cpp`. -* Ensure all tests pass with `make test` before submitting changes. +* Tests are located in `tests.cpp` using the `doctest` framework. +* Ensure all tests pass with `ctest --test-dir build --output-on-failure` (or `./build/tests`) before submitting changes. diff --git a/README.md b/README.md index fe0faf2..381f1de 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,17 @@ Looking for a C# version? Checkout the companion repository [astar-algorithm-csh ### Release notes +[v1.3.1](https://github.com/justinhj/astar-algorithm-cpp/releases/tag/v1.3.1) +Bug fixes, safety hardening, and codebase modernization: +- Guarded `FreeSolutionNodes()` against failed or uninitialized searches to eliminate potential use-after-free. +- Updated `~FixedSizeAllocator` to properly invoke destructors on live objects, avoiding resource leaks when states hold non-trivial members. +- Added double-free, alignment, and bounds validation to `FixedSizeAllocator::free()`. +- Fixed 64-bit pointer format specifiers (`%p`) in `fsa.h` `Debug()`. +- Ensured goal node heuristic (`h`) and total cost (`f`) are properly populated upon search success. +- Replaced legacy `NULL` and `0` pointer literals with C++11 `nullptr` across all headers. +- Removed unused `AStarState` dead code and retired `.travis.yml`. +- Secured `doctest` download with SHA256 `URL_HASH` in CMake. + [v1.3](https://github.com/justinhj/astar-algorithm-cpp/releases/tag/v1.3) Performance optimizations for the open list and addition of a reproducible benchmark suite: - Open list state membership lookup is now O(1) using an `unordered_set`, eliminating the previous O(N) linear search per successor. diff --git a/fsa.h b/fsa.h index 821a982..8a3010e 100644 --- a/fsa.h +++ b/fsa.h @@ -45,6 +45,8 @@ given where due. #ifndef FSA_H #define FSA_H +#include +#include #include #include @@ -62,11 +64,15 @@ class FixedSizeAllocator { FSA_ELEMENT* pPrev; FSA_ELEMENT* pNext; + bool bAllocated; }; public: // methods FixedSizeAllocator(unsigned int MaxElements = FSA_DEFAULT_SIZE) - : m_pFirstUsed(NULL), m_MaxElements(MaxElements) { + : m_pFirstFree(nullptr), + m_pFirstUsed(nullptr), + m_MaxElements(MaxElements), + m_pMemory(nullptr) { // Allocate enough memory for the maximum number of elements char* pMem = new char[m_MaxElements * sizeof(FSA_ELEMENT)]; @@ -86,27 +92,40 @@ class FixedSizeAllocator { for (unsigned int i = 0; i < m_MaxElements; i++) { pElement->pPrev = pElement - 1; pElement->pNext = pElement + 1; + pElement->bAllocated = false; pElement++; } // first element should have a null prev - m_pFirstFree->pPrev = NULL; + m_pFirstFree->pPrev = nullptr; // last element should have a null next - (pElement - 1)->pNext = NULL; + (pElement - 1)->pNext = nullptr; } ~FixedSizeAllocator() { + // Destroy any live objects remaining on the used list + FSA_ELEMENT* pNode = m_pFirstUsed; + while (pNode) { + FSA_ELEMENT* pNext = pNode->pNext; + pNode->UserType.~USER_TYPE(); + pNode->bAllocated = false; + pNode = pNext; + } + m_pFirstUsed = nullptr; + // Free up the memory delete[] (char*)m_pMemory; + m_pMemory = nullptr; + m_pFirstFree = nullptr; } // Allocate a new USER_TYPE and return a pointer to it USER_TYPE* alloc() { - FSA_ELEMENT* pNewNode = NULL; + FSA_ELEMENT* pNewNode = nullptr; if (!m_pFirstFree) { - return NULL; + return nullptr; } else { pNewNode = m_pFirstFree; m_pFirstFree = pNewNode->pNext; @@ -114,34 +133,51 @@ class FixedSizeAllocator { // if the new node points to another free node then // change that nodes prev free pointer... if (pNewNode->pNext) { - pNewNode->pNext->pPrev = NULL; + pNewNode->pNext->pPrev = nullptr; } // node is now on the used list - pNewNode->pPrev = NULL; // the allocated node is always first in the list + pNewNode->pPrev = nullptr; // the allocated node is always first in the list - if (m_pFirstUsed == NULL) { - pNewNode->pNext = NULL; // no other nodes + if (m_pFirstUsed == nullptr) { + pNewNode->pNext = nullptr; // no other nodes } else { m_pFirstUsed->pPrev = pNewNode; // insert this at the head of the used list pNewNode->pNext = m_pFirstUsed; } m_pFirstUsed = pNewNode; + pNewNode->bAllocated = true; } return reinterpret_cast(pNewNode); } // Free the given user type - // For efficiency I don't check whether the user_data is a valid - // pointer that was allocated. I may add some debug only checking - // (To add the debug check you'd need to make sure the pointer is in - // the m_pMemory area and is pointing at the start of a node) + // Guarded against invalid pointer, out of bounds, and double-free void free(USER_TYPE* user_data) { + if (!user_data) { + return; + } + FSA_ELEMENT* pNode = reinterpret_cast(user_data); + // Verify the pointer was allocated from this allocator + assert(pNode >= m_pMemory && pNode < m_pMemory + m_MaxElements); + assert(((uintptr_t)((char*)pNode - (char*)m_pMemory) % sizeof(FSA_ELEMENT)) == 0); + if (pNode < m_pMemory || pNode >= m_pMemory + m_MaxElements || + ((uintptr_t)((char*)pNode - (char*)m_pMemory) % sizeof(FSA_ELEMENT)) != 0) { + return; + } + + // Guard against double-free + assert(pNode->bAllocated); + if (!pNode->bAllocated) { + return; + } + pNode->bAllocated = false; + // manage used list, remove this node from it if (pNode->pPrev) { pNode->pPrev->pNext = pNode->pNext; @@ -155,11 +191,11 @@ class FixedSizeAllocator { } // add to free list - if (m_pFirstFree == NULL) { + if (m_pFirstFree == nullptr) { // free list was empty m_pFirstFree = pNode; - pNode->pPrev = NULL; - pNode->pNext = NULL; + pNode->pPrev = nullptr; + pNode->pNext = nullptr; } else { // Add this node at the start of the free list m_pFirstFree->pPrev = pNode; @@ -174,7 +210,7 @@ class FixedSizeAllocator { FSA_ELEMENT* p = m_pFirstFree; while (p) { - printf("%x!%x ", p->pPrev, p->pNext); + printf("%p!%p ", (void*)p->pPrev, (void*)p->pNext); p = p->pNext; } printf("\n"); @@ -183,7 +219,7 @@ class FixedSizeAllocator { p = m_pFirstUsed; while (p) { - printf("%x!%x ", p->pPrev, p->pNext); + printf("%p!%p ", (void*)p->pPrev, (void*)p->pNext); p = p->pNext; } printf("\n"); @@ -196,6 +232,9 @@ class FixedSizeAllocator { } USER_TYPE* GetNext(USER_TYPE* node) { + if (!node) { + return nullptr; + } return reinterpret_cast((reinterpret_cast(node))->pNext); } diff --git a/stlastar.h b/stlastar.h index 14c030b..80587e0 100644 --- a/stlastar.h +++ b/stlastar.h @@ -49,9 +49,6 @@ given where due. #pragma warning(disable : 4786) #endif -template -class AStarState; - // The AStar search class. UserState is the users state space type template class AStarSearch { @@ -80,7 +77,13 @@ class AStarSearch { size_t heap_index; // index in m_OpenList, or SIZE_MAX when not on the heap - Node() : parent(0), child(0), g(0.0f), h(0.0f), f(0.0f), heap_index(SIZE_MAX) {} + Node() + : parent(nullptr), + child(nullptr), + g(0.0f), + h(0.0f), + f(0.0f), + heap_index(SIZE_MAX) {} bool operator==(const Node& otherNode) const { return this->m_UserState.IsSameState(otherNode.m_UserState); @@ -103,22 +106,26 @@ class AStarSearch { // constructor just initialises private data AStarSearch() : m_State(SEARCH_STATE_NOT_INITIALISED), - m_CurrentSolutionNode(NULL), + m_CurrentSolutionNode(nullptr), #if USE_FSA_MEMORY m_FixedSizeAllocator(1000), #endif m_AllocateNodeCount(0), - m_CancelRequest(false) { + m_CancelRequest(false), + m_Start(nullptr), + m_Goal(nullptr) { } AStarSearch(int MaxNodes) : m_State(SEARCH_STATE_NOT_INITIALISED), - m_CurrentSolutionNode(NULL), + m_CurrentSolutionNode(nullptr), #if USE_FSA_MEMORY m_FixedSizeAllocator(MaxNodes), #endif m_AllocateNodeCount(0), - m_CancelRequest(false) { + m_CancelRequest(false), + m_Start(nullptr), + m_Goal(nullptr) { } // call at any time to cancel the search and free up all the memory @@ -133,7 +140,7 @@ class AStarSearch { m_Start = AllocateNode(); m_Goal = AllocateNode(); - assert((m_Start != NULL && m_Goal != NULL)); + assert((m_Start != nullptr && m_Goal != nullptr)); m_Start->m_UserState = Start; m_Goal->m_UserState = Goal; @@ -146,7 +153,7 @@ class AStarSearch { m_Start->g = 0; m_Start->h = m_Start->m_UserState.GoalDistanceEstimate(m_Goal->m_UserState); m_Start->f = m_Start->g + m_Start->h; - m_Start->parent = 0; + m_Start->parent = nullptr; // Push the start node on the Open list @@ -197,9 +204,11 @@ class AStarSearch { // Check for the goal, once we pop that we're done if (n->m_UserState.IsGoal(m_Goal->m_UserState)) { // The user is going to use the Goal Node he passed in - // so copy the parent pointer of n + // so copy the parent pointer and costs of n m_Goal->parent = n->parent; m_Goal->g = n->g; + m_Goal->h = n->h; + m_Goal->f = n->f; // A special case is that the goal was passed in as the start state // so handle that here @@ -236,7 +245,7 @@ class AStarSearch { // User provides this functions and uses AddSuccessor to add each successor of // node 'n' to m_Successors bool ret = - n->m_UserState.GetSuccessors(this, n->parent ? &n->parent->m_UserState : NULL); + n->m_UserState.GetSuccessors(this, n->parent ? &n->parent->m_UserState : nullptr); if (!ret) { typename std::vector::iterator successor; @@ -405,6 +414,10 @@ class AStarSearch { // This is done to clean up all used Node memory when you are done with the // search void FreeSolutionNodes() { + if (m_State != SEARCH_STATE_SUCCEEDED || m_Start == nullptr) { + return; + } + Node* n = m_Start; if (m_Start->child) { @@ -413,7 +426,7 @@ class AStarSearch { n = n->child; FreeNode(del); - del = NULL; + del = nullptr; } while (n != m_Goal); @@ -425,6 +438,9 @@ class AStarSearch { FreeNode(m_Start); FreeNode(m_Goal); } + + m_Start = nullptr; + m_Goal = nullptr; } // Functions for traversing the solution @@ -435,7 +451,7 @@ class AStarSearch { if (m_Start) { return &m_Start->m_UserState; } else { - return NULL; + return nullptr; } } @@ -451,7 +467,7 @@ class AStarSearch { } } - return NULL; + return nullptr; } // Get end node @@ -460,7 +476,7 @@ class AStarSearch { if (m_Goal) { return &m_Goal->m_UserState; } else { - return NULL; + return nullptr; } } @@ -476,7 +492,7 @@ class AStarSearch { } } - return NULL; + return nullptr; } // Get final cost of solution @@ -506,7 +522,7 @@ class AStarSearch { return &(*iterDbgOpen)->m_UserState; } - return NULL; + return nullptr; } UserState* GetOpenListNext() { @@ -523,7 +539,7 @@ class AStarSearch { return &(*iterDbgOpen)->m_UserState; } - return NULL; + return nullptr; } UserState* GetClosedListStart() { @@ -541,7 +557,7 @@ class AStarSearch { return &(*iterDbgClosed)->m_UserState; } - return NULL; + return nullptr; } UserState* GetClosedListNext() { @@ -559,7 +575,7 @@ class AStarSearch { return &(*iterDbgClosed)->m_UserState; } - return NULL; + return nullptr; } // Get the number of steps @@ -667,6 +683,9 @@ class AStarSearch { // delete the goal FreeNode(m_Goal); + + m_Start = nullptr; + m_Goal = nullptr; } // This call is made by the search class when the search ends. A lot of nodes may be @@ -683,7 +702,7 @@ class AStarSearch { if (!n->child) { FreeNode(n); - n = NULL; + n = nullptr; } iterOpen++; @@ -700,7 +719,7 @@ class AStarSearch { if (!n->child) { FreeNode(n); - n = NULL; + n = nullptr; } } @@ -717,7 +736,7 @@ class AStarSearch { Node* address = m_FixedSizeAllocator.alloc(); if (!address) { - return NULL; + return nullptr; } m_AllocateNodeCount++; Node* p = new (address) Node; @@ -786,21 +805,4 @@ class AStarSearch { bool m_CancelRequest; }; -template -class AStarState { - public: - virtual ~AStarState() {} - virtual float GoalDistanceEstimate( - T& nodeGoal) = 0; // Heuristic function which computes the estimated cost to the goal node - virtual bool IsGoal(T& nodeGoal) = 0; // Returns true if this node is the goal node - virtual bool GetSuccessors( - AStarSearch* astarsearch, - T* parent_node) = 0; // Retrieves all successors to this node and adds them via - // astarsearch.addSuccessor() - virtual float GetCost( - T& successor) = 0; // Computes the cost of travelling from this node to the successor node - virtual bool IsSameState(T& rhs) = 0; // Returns true if this node is the same as the rhs node - virtual size_t Hash() = 0; // Returns a hash for the state -}; - #endif diff --git a/tests.cpp b/tests.cpp index 4f30142..b81b9f7 100644 --- a/tests.cpp +++ b/tests.cpp @@ -217,3 +217,103 @@ TEST_CASE("Map Search") { CHECK(SearchSteps == 227); } } + +TEST_CASE("FreeSolutionNodes After Failed Search") { + AStarSearch astarsearch; + + MapSearchNode nodeStart(0, 0); + // (1, 1) has terrain value 9 (obstacle), which can never be reached + MapSearchNode nodeEnd(1, 1); + + astarsearch.SetStartAndGoalStates(nodeStart, nodeEnd); + + unsigned int SearchState; + do { + SearchState = astarsearch.SearchStep(); + } while (SearchState == AStarSearch::SEARCH_STATE_SEARCHING); + + CHECK(SearchState == AStarSearch::SEARCH_STATE_FAILED); + + // Calling FreeSolutionNodes after a failed search must be safe and not cause use-after-free + astarsearch.FreeSolutionNodes(); + // Idempotent call + astarsearch.FreeSolutionNodes(); + + astarsearch.EnsureMemoryFreed(); +} + +TEST_CASE("Goal Node Costs Populated On Success") { + AStarSearch astarsearch; + + MapSearchNode nodeStart(0, 0); + // (5, 0) is along row 0 and is reachable from (0, 0) + MapSearchNode nodeEnd(5, 0); + + astarsearch.SetStartAndGoalStates(nodeStart, nodeEnd); + + unsigned int SearchState; + do { + SearchState = astarsearch.SearchStep(); + } while (SearchState == AStarSearch::SEARCH_STATE_SEARCHING); + + CHECK(SearchState == AStarSearch::SEARCH_STATE_SUCCEEDED); + CHECK(astarsearch.GetSolutionCost() > 0.0f); + + MapSearchNode* goalNode = astarsearch.GetSolutionEnd(); + REQUIRE(goalNode != nullptr); + CHECK(goalNode->x == 5); + CHECK(goalNode->y == 0); + + astarsearch.FreeSolutionNodes(); + astarsearch.EnsureMemoryFreed(); +} + +struct DestructorTracker { + static int alive_count; + DestructorTracker() { alive_count++; } + ~DestructorTracker() { alive_count--; } +}; +int DestructorTracker::alive_count = 0; + +TEST_CASE("FixedSizeAllocator Destructor Cleans Up Live Objects") { + DestructorTracker::alive_count = 0; + { + FixedSizeAllocator allocator(10); + DestructorTracker* a = allocator.alloc(); + new (a) DestructorTracker(); + + DestructorTracker* b = allocator.alloc(); + new (b) DestructorTracker(); + + DestructorTracker* c = allocator.alloc(); + new (c) DestructorTracker(); + + CHECK(DestructorTracker::alive_count == 3); + + // Manually destroy and free 'b' + b->~DestructorTracker(); + allocator.free(b); + CHECK(DestructorTracker::alive_count == 2); + + // 'a' and 'c' are still alive in the allocator. + // When allocator goes out of scope, ~FixedSizeAllocator must destroy 'a' and 'c'. + } + CHECK(DestructorTracker::alive_count == 0); +} + +TEST_CASE("FixedSizeAllocator Guard Against Double Free") { + FixedSizeAllocator allocator(5); + int* p = allocator.alloc(); + CHECK(p != nullptr); + + allocator.free(p); + + // Freeing nullptr should be safe + allocator.free(nullptr); + + // After freeing, allocating again should work normally + int* p2 = allocator.alloc(); + CHECK(p2 != nullptr); + allocator.free(p2); +} +