-
Notifications
You must be signed in to change notification settings - Fork 298
CWE-407: 12 O(N²) algorithmic complexity fixes — HashSet/HashMap shadow indexes #1237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
3882203
95c41c2
9e92d56
14b5a78
d20c160
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove all of the // build HashSet for O(1) lookupsDo you mind going through and updating or removing the rest of your comments across this PR please?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the curly braces here? |
||
| bones_to_process = src_skeleton->get_parentless_bones(); | ||
| { | ||
| TypedArray<Node> nodes = p_base_scene->find_children("*", "AnimationPlayer"); | ||
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
| 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); | ||
|
|
@@ -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)); | ||
|
|
@@ -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) { | ||
|
|
@@ -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); | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: Redot-Engine/redot-engine
Length of output: 3083
🏁 Script executed:
Repository: Redot-Engine/redot-engine
Length of output: 1996
🏁 Script executed:
Repository: Redot-Engine/redot-engine
Length of output: 224
open_indexmust be maintained when points enter and leave theopen_listheap.Line 591 calls
push_heap(..., e->open_index, ...)for existing-open nodes, butAStarGrid2D::_solvenever setsopen_indexwhen a point is added toopen_list(line 574), nor resets it when removed (lines ~548–549). This leavesopen_indexas-1or 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