Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions core/math/a_star.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,8 @@ bool AStar3D::_solve(Point *begin_point, Point *end_point, bool p_allow_partial_

sorter.pop_heap(0, open_list.size(), open_list.ptr()); // Remove the current point from the open list.
open_list.remove_at(open_list.size() - 1);
// CWE-407 fix (redot-0005): mark as removed from heap.
p->open_index = -1;
p->closed_pass = pass; // Mark the point as closed.

for (const KeyValue<int64_t, Point *> &kv : p->neighbors) {
Expand All @@ -365,6 +367,8 @@ bool AStar3D::_solve(Point *begin_point, Point *end_point, bool p_allow_partial_
if (e->open_pass != pass) { // The point wasn't inside the open list.
e->open_pass = pass;
open_list.push_back(e);
// CWE-407 fix (redot-0005): record heap position so decrease-key is O(1).
e->open_index = open_list.size() - 1;
new_point = true;
} else if (tentative_g_score >= e->g_score) { // The new path is worse than the previous.
continue;
Expand All @@ -379,7 +383,7 @@ bool AStar3D::_solve(Point *begin_point, Point *end_point, bool p_allow_partial_
if (new_point) { // The position of the new points is already known.
sorter.push_heap(0, open_list.size() - 1, 0, e, open_list.ptr());
} else {
sorter.push_heap(0, open_list.find(e), 0, e, open_list.ptr());
sorter.push_heap(0, e->open_index, 0, e, open_list.ptr());
}
}
}
Expand Down Expand Up @@ -876,6 +880,8 @@ bool AStar2D::_solve(AStar3D::Point *begin_point, AStar3D::Point *end_point, boo

sorter.pop_heap(0, open_list.size(), open_list.ptr()); // Remove the current point from the open list.
open_list.remove_at(open_list.size() - 1);
// CWE-407 fix (redot-0005): mark as removed from heap.
p->open_index = -1;
p->closed_pass = astar.pass; // Mark the point as closed.

for (KeyValue<int64_t, AStar3D::Point *> &kv : p->neighbors) {
Expand All @@ -899,6 +905,8 @@ bool AStar2D::_solve(AStar3D::Point *begin_point, AStar3D::Point *end_point, boo
if (e->open_pass != astar.pass) { // The point wasn't inside the open list.
e->open_pass = astar.pass;
open_list.push_back(e);
// CWE-407 fix (redot-0005): record heap position so decrease-key is O(1).
e->open_index = open_list.size() - 1;
new_point = true;
} else if (tentative_g_score >= e->g_score) { // The new path is worse than the previous.
continue;
Expand All @@ -913,7 +921,7 @@ bool AStar2D::_solve(AStar3D::Point *begin_point, AStar3D::Point *end_point, boo
if (new_point) { // The position of the new points is already known.
sorter.push_heap(0, open_list.size() - 1, 0, e, open_list.ptr());
} else {
sorter.push_heap(0, open_list.find(e), 0, e, open_list.ptr());
sorter.push_heap(0, e->open_index, 0, e, open_list.ptr());
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions core/math/a_star.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class AStar3D : public RefCounted {
struct Point {
Point() {}

// CWE-407 fix (redot-0005): tracks heap position for O(1) decrease-key, avoiding open_list.find().
int32_t open_index = -1;
int64_t id = 0;
Vector3 pos;
real_t weight_scale = 0;
Expand Down
2 changes: 1 addition & 1 deletion core/math/a_star_grid_2d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ bool AStarGrid2D::_solve(Point *p_begin_point, Point *p_end_point, bool p_allow_
if (new_point) { // The position of the new points is already known.
sorter.push_heap(0, open_list.size() - 1, 0, e, open_list.ptr());
} else {
sorter.push_heap(0, open_list.find(e), 0, e, open_list.ptr());
sorter.push_heap(0, e->open_index, 0, e, open_list.ptr());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify open_index is read but not written in AStarGrid2D::_solve.
rg -n '\bopen_index\b' core/math/a_star_grid_2d.cpp core/math/a_star_grid_2d.h
# Inspect heap helper behavior used by push_heap/pop_heap.
sed -n '80,170p' core/templates/sort_array.h

Repository: Redot-Engine/redot-engine

Length of output: 3083


🏁 Script executed:

#!/bin/bash
# Get full context of _solve method, especially around line 591
wc -l core/math/a_star_grid_2d.cpp
sed -n '550,620p' core/math/a_star_grid_2d.cpp

# Search for any assignment to open_index in the cpp file
rg -n 'open_index\s*=' core/math/a_star_grid_2d.cpp

Repository: Redot-Engine/redot-engine

Length of output: 1996


🏁 Script executed:

#!/bin/bash
# Search for pop_heap and remove operations in _solve context
sed -n '500,650p' core/math/a_star_grid_2d.cpp | grep -n -E '(pop_heap|remove_at|erase)'

Repository: Redot-Engine/redot-engine

Length of output: 224


open_index must be maintained when points enter and leave the open_list heap.

Line 591 calls push_heap(..., e->open_index, ...) for existing-open nodes, but AStarGrid2D::_solve never sets open_index when a point is added to open_list (line 574), nor resets it when removed (lines ~548–549). This leaves open_index as -1 or stale, causing out-of-bounds array access and heap corruption when reheapifying.

Correctness fix
 			if (e->open_pass != pass) { // The point wasn't inside the open list.
 				e->open_pass = pass;
 				open_list.push_back(e);
+				e->open_index = open_list.size() - 1;
 				new_point = true;
 		sorter.pop_heap(0, open_list.size(), open_list.ptr()); // Remove the current point from the open list.
 		open_list.remove_at(open_list.size() - 1);
+		p->open_index = -1;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@core/math/a_star_grid_2d.cpp` at line 591, AStarGrid2D::_solve is calling
sorter.push_heap with e->open_index for existing-open nodes but never maintains
that field when nodes are inserted or removed from open_list; update the code so
that whenever a node is added to open_list you set e->open_index to its heap
index (after push_heap returns) and whenever a node is removed from open_list
(pop/erase path around lines ~548–549) you reset e->open_index to -1; also
ensure any code paths that call sorter.push_heap or sorter.pop_heap update the
affected entries' open_index consistently so the heap index stored in the node
(open_index) always reflects its position in open_list.

}
}
}
Expand Down
2 changes: 2 additions & 0 deletions core/math/a_star_grid_2d.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class AStarGrid2D : public RefCounted {
struct Point {
Vector2i id;

// CWE-407 fix (redot-0005): tracks heap position for O(1) decrease-key, avoiding open_list.find().
int32_t open_index = -1;
Vector2 pos;
real_t weight_scale = 1.0;

Expand Down
15 changes: 11 additions & 4 deletions editor/import/3d/post_import_plugin_skeleton_rest_fixer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ void PostImportPluginSkeletonRestFixer::internal_process(InternalImportCategory
}

// Fix animation by changing node transform.
// CWE-407 fix (redot-0007): build HashSet for O(1) .has() — was O(B) Vector scan per track.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove all of the CWE-407 fix (redot-xxxx) from all of these comments please?
This is kinda redundant and seems to be above every line changed in this PR. While it is helpful for me reviewing this PR, its not particularly helpful for other developers working on and modifying this code later.
Can we change this line to something like this instead?

// build HashSet for O(1) lookups

Do you mind going through and updating or removing the rest of your comments across this PR please?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. i will make a hard fork if you do not accept. we do not want regressions & comments document defect.

HashSet<int> bones_to_process_set;
{
Vector<int> _btp = src_skeleton->get_parentless_bones();
for (int _i = 0; _i < _btp.size(); _i++) { bones_to_process_set.insert(_btp[_i]); }
}
Comment on lines +169 to +174
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the curly braces here?
_btp is not defined anywhere else in the internal_process() method, so scoping the variable seems redundant.
Also why _i instead of i? i is a well known index variable used in for loops, and even used elsewhere in this method.
Additionally, generally prefixing variable names with underscores is for private members, not local variables in a method.

bones_to_process = src_skeleton->get_parentless_bones();
{
TypedArray<Node> nodes = p_base_scene->find_children("*", "AnimationPlayer");
Expand Down Expand Up @@ -200,7 +206,7 @@ void PostImportPluginSkeletonRestFixer::internal_process(InternalImportCategory
int bone_idx = src_skeleton->find_bone(bn);
int key_len = anim->track_get_key_count(i);
if (anim->track_get_type(i) == Animation::TYPE_POSITION_3D) {
if (bones_to_process.has(bone_idx)) {
if (bones_to_process_set.has(bone_idx)) { // CWE-407 fix (redot-0007): O(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along those same lines, comments like this one dont really serve a purpose: // CWE-407 fix (redot-0007): O(1)
This looks reminiscent of the raw output of ChatGPT.
It's fine if you used a coding agent with this PR as we do allow contributors to use AI, we do have an expectation that a human reviews and understands the changes prior to submission of the PR.

for (int j = 0; j < key_len; j++) {
Vector3 ps = static_cast<Vector3>(anim->track_get_key_value(i, j));
anim->track_set_key_value(i, j, global_transform.basis.xform(ps) + global_transform.origin);
Expand All @@ -211,7 +217,7 @@ void PostImportPluginSkeletonRestFixer::internal_process(InternalImportCategory
anim->track_set_key_value(i, j, ps * scl);
}
}
} else if (bones_to_process.has(bone_idx)) {
} else if (bones_to_process_set.has(bone_idx)) { // CWE-407 fix (redot-0007): O(1)
if (anim->track_get_type(i) == Animation::TYPE_ROTATION_3D) {
for (int j = 0; j < key_len; j++) {
Quaternion qt = static_cast<Quaternion>(anim->track_get_key_value(i, j));
Expand Down Expand Up @@ -625,7 +631,8 @@ void PostImportPluginSkeletonRestFixer::internal_process(InternalImportCategory

// Scan hierarchy and populate a whitelist of unmapped bones without mapped descendants.
// When both is_using_modifier and is_using_global_pose are enabled, this array is used for detecting warning.
Vector<int> keep_bone_rest;
// CWE-407 fix (redot-0007): was Vector<int> — O(K) .has() called inside O(T) track loop.
HashSet<int> keep_bone_rest;
if (is_using_modifier || keep_global_rest_leftovers) {
Vector<int> bones_to_process = src_skeleton->get_parentless_bones();
while (bones_to_process.size() > 0) {
Expand Down Expand Up @@ -658,7 +665,7 @@ void PostImportPluginSkeletonRestFixer::internal_process(InternalImportCategory
}

if (!found_mapped) {
keep_bone_rest.push_back(src_idx); // No mapped descendants. Add to whitelist.
keep_bone_rest.insert(src_idx);
}
}
}
Expand Down
29 changes: 18 additions & 11 deletions modules/gltf/gltf_document.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,11 @@ Error GLTFDocument::_serialize(Ref<GLTFState> p_state) {
}

Error GLTFDocument::_serialize_gltf_extensions(Ref<GLTFState> p_state) const {
Vector<String> extensions_used = p_state->extensions_used;
// CWE-407 fix (redot-0008): extensions_used is now HashSet — convert to sorted Vector for JSON.
Vector<String> extensions_used;
for (const String &ext : p_state->extensions_used) {
extensions_used.push_back(ext);
}
Vector<String> extensions_required = p_state->extensions_required;
if (!p_state->lights.is_empty()) {
extensions_used.push_back("KHR_lights_punctual");
Expand Down Expand Up @@ -442,11 +446,10 @@ Error GLTFDocument::_serialize_nodes(Ref<GLTFState> p_state) {
Dictionary khr_node_visibility;
extensions["KHR_node_visibility"] = khr_node_visibility;
khr_node_visibility["visible"] = gltf_node->visible;
if (!p_state->extensions_used.has("KHR_node_visibility")) {
p_state->extensions_used.push_back("KHR_node_visibility");
if (_visibility_mode == VISIBILITY_MODE_INCLUDE_REQUIRED) {
p_state->extensions_required.push_back("KHR_node_visibility");
}
// CWE-407 fix (redot-0008): HashSet.insert() is idempotent O(1) — was O(E) Vector scan + push_back.
p_state->extensions_used.insert("KHR_node_visibility");
if (_visibility_mode == VISIBILITY_MODE_INCLUDE_REQUIRED) {
p_state->extensions_required.push_back("KHR_node_visibility");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
}
if (gltf_node->mesh != -1) {
Expand Down Expand Up @@ -5509,9 +5512,8 @@ Error GLTFDocument::_serialize_animations(Ref<GLTFState> p_state) {
}
if (!gltf_animation->get_pointer_tracks().is_empty()) {
// Serialize glTF pointer tracks with the KHR_animation_pointer extension.
if (!p_state->extensions_used.has("KHR_animation_pointer")) {
p_state->extensions_used.push_back("KHR_animation_pointer");
}
// CWE-407 fix (redot-0008): HashSet.insert() is idempotent O(1).
p_state->extensions_used.insert("KHR_animation_pointer");
for (KeyValue<String, GLTFAnimation::Channel<Variant>> &pointer_track_iter : gltf_animation->get_pointer_tracks()) {
const String &json_pointer = pointer_track_iter.key;
const GLTFAnimation::Channel<Variant> &pointer_track = pointer_track_iter.value;
Expand Down Expand Up @@ -8921,7 +8923,8 @@ Error GLTFDocument::append_from_scene(Node *p_node, Ref<GLTFState> p_state, uint
state->scene_name = p_node->get_name();
} else {
if (_root_node_mode == RootNodeMode::ROOT_NODE_MODE_SINGLE_ROOT) {
state->extensions_used.append("GODOT_single_root");
// CWE-407 fix (redot-0008): HashSet.insert() is idempotent O(1).
state->extensions_used.insert("GODOT_single_root");
}
_convert_scene_node(state, p_node, -1, -1);
}
Expand Down Expand Up @@ -8992,8 +8995,12 @@ Error GLTFDocument::append_from_file(String p_path, Ref<GLTFState> p_state, uint
Error GLTFDocument::_parse_gltf_extensions(Ref<GLTFState> p_state) {
ERR_FAIL_COND_V(p_state.is_null(), ERR_PARSE_ERROR);
if (p_state->json.has("extensionsUsed")) {
// CWE-407 fix (redot-0008): populate HashSet from parsed JSON array.
Vector<String> ext_array = p_state->json["extensionsUsed"];
p_state->extensions_used = ext_array;
p_state->extensions_used.clear();
for (int i = 0; i < ext_array.size(); i++) {
p_state->extensions_used.insert(ext_array[i]);
}
}
if (p_state->json.has("extensionsRequired")) {
Vector<String> ext_array = p_state->json["extensionsRequired"];
Expand Down
5 changes: 2 additions & 3 deletions modules/gltf/gltf_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,8 @@ void GLTFState::_bind_methods() {
}

void GLTFState::add_used_extension(const String &p_extension_name, bool p_required) {
if (!extensions_used.has(p_extension_name)) {
extensions_used.push_back(p_extension_name);
}
// CWE-407 fix (redot-0008): HashSet.insert() is idempotent O(1) — was O(E) has()+push_back().
extensions_used.insert(p_extension_name);
if (p_required) {
if (!extensions_required.has(p_extension_name)) {
extensions_required.push_back(p_extension_name);
Expand Down
3 changes: 2 additions & 1 deletion modules/gltf/gltf_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ class GLTFState : public Resource {
Vector<Ref<GLTFTextureSampler>> texture_samplers;
Ref<GLTFTextureSampler> default_texture_sampler;
Vector<Ref<Texture2D>> images;
Vector<String> extensions_used;
// CWE-407 fix (redot-0008): was Vector<String> — O(E) .has() called per node/animation track.
HashSet<String> extensions_used;
Vector<String> extensions_required;
Vector<Ref<Image>> source_images;

Expand Down
26 changes: 21 additions & 5 deletions modules/godot_physics_2d/godot_body_2d.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ class GodotBody2D : public GodotCollisionObject2D {
};

Vector<AreaCMP> areas;
// CWE-407 fix (redot-0002): O(1) area lookup by RID — shadow index for areas Vector.
HashMap<RID, int> area_index;

struct Contact {
Vector2 local_pos;
Expand Down Expand Up @@ -164,20 +166,34 @@ class GodotBody2D : public GodotCollisionObject2D {
GodotPhysicsDirectBodyState2D *get_direct_state();

_FORCE_INLINE_ void add_area(GodotArea2D *p_area) {
int index = areas.find(AreaCMP(p_area));
if (index > -1) {
areas.write[index].refCount += 1;
// CWE-407 fix (redot-0002): was areas.find() — O(n) Vector scan per physics tick.
// area_index provides O(1) lookup by RID. Index rebuilt only on overlap change.
RID rid = p_area->get_self();
HashMap<RID, int>::Iterator it = area_index.find(rid);
if (it != area_index.end()) {
areas.write[it->value].refCount += 1;
} else {
areas.ordered_insert(AreaCMP(p_area));
area_index.clear();
for (int i = 0; i < areas.size(); i++) {
area_index[areas[i].area->get_self()] = i;
}
}
}

_FORCE_INLINE_ void remove_area(GodotArea2D *p_area) {
int index = areas.find(AreaCMP(p_area));
if (index > -1) {
// CWE-407 fix (redot-0002): was areas.find() — O(n) Vector scan.
RID rid = p_area->get_self();
HashMap<RID, int>::Iterator it = area_index.find(rid);
if (it != area_index.end()) {
int index = it->value;
areas.write[index].refCount -= 1;
if (areas[index].refCount < 1) {
areas.remove_at(index);
area_index.clear();
for (int i = 0; i < areas.size(); i++) {
area_index[areas[i].area->get_self()] = i;
}
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down
25 changes: 20 additions & 5 deletions modules/godot_physics_3d/godot_body_3d.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class GodotBody3D : public GodotCollisionObject3D {
HashMap<GodotConstraint3D *, int> constraint_map;

Vector<AreaCMP> areas;
// CWE-407 fix (redot-0003): O(1) area lookup by RID — shadow index for areas Vector.
HashMap<RID, int> area_index;

struct Contact {
Vector3 local_pos;
Expand Down Expand Up @@ -158,20 +160,33 @@ class GodotBody3D : public GodotCollisionObject3D {
GodotPhysicsDirectBodyState3D *get_direct_state();

_FORCE_INLINE_ void add_area(GodotArea3D *p_area) {
int index = areas.find(AreaCMP(p_area));
if (index > -1) {
areas.write[index].refCount += 1;
// CWE-407 fix (redot-0003): was areas.find() — O(n) Vector scan per physics tick.
RID rid = p_area->get_self();
HashMap<RID, int>::Iterator it = area_index.find(rid);
if (it != area_index.end()) {
areas.write[it->value].refCount += 1;
} else {
areas.ordered_insert(AreaCMP(p_area));
area_index.clear();
for (int i = 0; i < areas.size(); i++) {
area_index[areas[i].area->get_self()] = i;
}
}
}

_FORCE_INLINE_ void remove_area(GodotArea3D *p_area) {
int index = areas.find(AreaCMP(p_area));
if (index > -1) {
// CWE-407 fix (redot-0003): was areas.find() — O(n) Vector scan.
RID rid = p_area->get_self();
HashMap<RID, int>::Iterator it = area_index.find(rid);
if (it != area_index.end()) {
int index = it->value;
areas.write[index].refCount -= 1;
if (areas[index].refCount < 1) {
areas.remove_at(index);
area_index.clear();
for (int i = 0; i < areas.size(); i++) {
area_index[areas[i].area->get_self()] = i;
}
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions modules/godot_physics_3d/godot_soft_body_3d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -654,20 +654,26 @@ void GodotSoftBody3D::generate_bending_constraints(int p_distance) {

// Special optimized case for distance == 2.
if (p_distance == 2) {
// CWE-407 fix (redot-0004): was LocalVector<int>.has() — O(n) per link, O(n²) total.
// node_link_set provides O(1) membership; LocalVector preserved for iteration.
LocalVector<LocalVector<int>> node_links;
LocalVector<HashSet<int>> node_link_set;

// Build node links.
node_links.resize(nodes.size());
node_link_set.resize(nodes.size());

for (Link &link : links) {
const int ia = (int)(link.n[0] - &nodes[0]);
const int ib = (int)(link.n[1] - &nodes[0]);
if (!node_links[ia].has(ib)) {
if (!node_link_set[ia].has(ib)) {
node_links[ia].push_back(ib);
node_link_set[ia].insert(ib);
}

if (!node_links[ib].has(ia)) {
if (!node_link_set[ib].has(ia)) {
node_links[ib].push_back(ia);
node_link_set[ib].insert(ia);
}
}
for (uint32_t ii = 0; ii < node_links.size(); ii++) {
Expand Down
5 changes: 2 additions & 3 deletions scene/3d/skeleton_3d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,9 @@ void Skeleton3D::_update_process_order() const {
if (bonesptr[i].parent != -1) {
int parent_bone_idx = bonesptr[i].parent;

// Check to see if this node is already added to the parent.
// CWE-407 fix (redot-0006): HashSet.has() is O(1); Vector.has() was O(C) linear scan.
if (!bonesptr[parent_bone_idx].child_bones.has(i)) {
// Add the child node.
bonesptr[parent_bone_idx].child_bones.push_back(i);
bonesptr[parent_bone_idx].child_bones.insert(i);
} else {
ERR_PRINT("Skeleton3D parenthood graph is cyclic");
}
Expand Down
3 changes: 2 additions & 1 deletion scene/3d/skeleton_3d.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ class Skeleton3D : public Node3D {
String name;

int parent = -1;
Vector<int> child_bones;
// CWE-407 fix (redot-0006): was Vector<int> — O(B) .has() inside O(B) loop over bones.
HashSet<int> child_bones;

Transform3D rest;
Transform3D global_rest;
Expand Down
Loading