Skip to content

Crash instead of returning an invalid index value#166

Merged
DanielaOrtner merged 1 commit into
RebelToolbox:mainfrom
madmiraal:fix-bvh-array-out-of-bounds
May 5, 2025
Merged

Crash instead of returning an invalid index value#166
DanielaOrtner merged 1 commit into
RebelToolbox:mainfrom
madmiraal:fix-bvh-array-out-of-bounds

Conversation

@madmiraal
Copy link
Copy Markdown
Contributor

Currently, when a new BVH leaf item is requested, if the BVH leaf is full, a value of -1 is returned.
The compiler is raising a array subscript 4294967295 is above array bounds warning when this value is used as an index in an array of fixed size:

In file included from ./core/math/bvh_tree.h:168,
                 from ./core/math/bvh.h:25,
                 from servers/physics_2d/broad_phase_2d_bvh.h:11,
                 from servers/physics_2d/broad_phase_2d_bvh.cpp:7:
In member function 'BVH_ABB<BOUNDS, POINT>& BVH_Tree<T, MAX_CHILDREN, MAX_ITEMS, USE_PAIRS, BOUNDS, POINT>::TLeaf::get_aabb(uint32_t) [with T = CollisionObject2DSW; int MAX_CHILDREN = 2; int MAX_ITEMS = 128; bool USE_PAIRS = true; BOUNDS = Rect2; POINT = Vector2]',
    inlined from 'bool BVH_Tree<T, MAX_CHILDREN, MAX_ITEMS, USE_PAIRS, BOUNDS, POINT>::_node_add_item(uint32_t, uint32_t, const BVH_ABB<BOUNDS, POINT>&) [with T = CollisionObject2DSW; int MAX_CHILDREN = 2; int MAX_ITEMS = 128; bool USE_PAIRS = true; BOUNDS = Rect2; POINT = Vector2]' at ./core/math/bvh_tree.h:439:22,
    inlined from 'BVHHandle BVH_Tree<T, MAX_CHILDREN, MAX_ITEMS, USE_PAIRS, BOUNDS, POINT>::item_add(T*, bool, const BOUNDS&, int32_t, bool, uint32_t, uint32_t, bool) [with T = CollisionObject2DSW; int MAX_CHILDREN = 2; int MAX_ITEMS = 128; bool USE_PAIRS = true; BOUNDS = Rect2; POINT = Vector2]' at ./core/math/bvh_public.inc:84:36,
    inlined from 'BVHHandle BVH_Manager<T, USE_PAIRS, MAX_ITEMS, BOUNDS, POINT, BVH_THREAD_SAFE>::create(T*, bool, const BOUNDS&, int, bool, uint32_t, uint32_t) [with T = CollisionObject2DSW; bool USE_PAIRS = true; int MAX_ITEMS = 128; BOUNDS = Rect2; POINT = Vector2; bool BVH_THREAD_SAFE = true]' at ./core/math/bvh.h:119:36,
    inlined from 'virtual BroadPhase2DSW::ID BroadPhase2DBVH::create(CollisionObject2DSW*, int, const Rect2&, bool)' at servers/physics_2d/broad_phase_2d_bvh.cpp:18:24:
./core/math/bvh_structs.inc:48:21: error: array subscript 4294967295 is above array bounds of 'BVH_ABB<Rect2, Vector2> [128]' [-Werror=array-bounds=]
   48 |         return aabbs[p_id];
      |                ~~~~~^

The only time a leaf item is requested is when an item is added to a node, and, in debug builds, the returned value is checked and Rebel Engine will crash if it equals 0xffffffff (the overflowed value of -1 for an unsigned 32 bit integer is 4294967295):

ref.item_id = leaf.request_item();
BVH_ASSERT(ref.item_id != BVHCommon::INVALID);

static const uint32_t INVALID = (0xffffffff);

// debug only assert
#ifdef BVH_CHECKS
#define BVH_ASSERT(a) CRASH_COND((a) == false)
#else
#define BVH_ASSERT(a)
#endif

It appears that it is currently assumed that no attempt will be made to add an item to a node that is full (and that the node is a leaf node). Addressing the dependence on this assumption will require significant changes to the BVH code. In the meantime, it is easier to simply crash if any attempt is made to request an item from a full leaf node.

This PR cause Rebel Engine to crash whenever a leaf item is requested from a full leaf node; instead of only in debug builds.

@madmiraal madmiraal added the PR Type: Bug Fix Your current game should now work as expected. label Apr 30, 2025
@madmiraal madmiraal force-pushed the fix-bvh-array-out-of-bounds branch from 53cf854 to 2e40800 Compare April 30, 2025 17:47
@DanielaOrtner DanielaOrtner self-requested a review May 5, 2025 17:54
@DanielaOrtner DanielaOrtner merged commit 1c97b40 into RebelToolbox:main May 5, 2025
15 checks passed
@madmiraal madmiraal deleted the fix-bvh-array-out-of-bounds branch May 6, 2025 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR Type: Bug Fix Your current game should now work as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants