Fix crash when inserting keyframes with empty properties array

[TopherBriggs]

Fix crash when inserting keyframes with empty properties array

Bugsquad edit:

• Fixes (for 4.4) Crash when inserting rotation keyframe #98751 (Crash when inserting rotation keyframe)
  Doesn't seem to be the case, the issue just sounds related.
• Fixes Crash on clicking Animation track key. #99249.

Description

When inserting rotation keyframes in the animation editor, clicking on the keyframe would cause the editor to crash due to an Array out-of-bound error. Upon further inspection using coredumpctl it seems to be crashing from a out-of-bounds index on editor_inspector.cpp:2727, with accessing F.properties[0] without checking if the properties array is empty could cause a crash. This is accessed to see if the property is in the favorites list. However, if an element such as AnimationTrackKeyEdit does not have any properties that can be favorited. This PR adds a safety check to prevent accessing an empty array.

Modified Files

• editor/editor_inspector.cpp

Changes

The following changes prevent the crash by adding an empty check before accessing the properties array:

EditorProperty ***ep = Object::cast_to<EditorProperty>(F.property_editor);
- if (ep && current_favorites.has(F.properties[0])) {
+ if (ep && !F.properties.is_empty() && current_favorites.has(F.properties[0])) {
    ep->favorited = true;
    favorites_vbox->add_child(F.property_editor);
} else {
    current_vbox->add_child(F.property_editor);
}

Testing

Tested with the provided reproduction case:

1. Imported the FBX model from the reproduction case
2. Created a new inherited scene
3. Added animation track as child to Skeleton3D
4. Created new animation
5. Added upper_arm.L and upper_arm.R rotation to track
6. Successfully inserted keyframe without crash

Tested on:

• Godot v4.4.dev
• Arch Linux x86_64 GNOME 47.1 Kernel: 6.11.6-arch1-1
• CPU: 12th Gen Intel i7-1260P (16) @ 4.700GHz
• GPU: Intel Alder Lake-P GT2 [Iris Xe Graphics]

Notes

The crash occurred because the properties array could be empty when attempting to insert certain types of keyframes in the animation editor. This fix ensures we check for empty arrays before accessing their elements, following proper array bounds checking practices.

[TokageItLab]

Can you provide a video capture of the detailed reproduction procedure for #98751 first? Since the crash does not occur in my environment.

Also, the code you are trying to fix has been no longer exist in master. So check again if the problem occurs on the latest master branch. Sorry I was watching a delayed brunch. In any case, #98751 is reported in 4.3 stable (It means that the code you are trying to fix does not exist in 4.3 stable, if this PR is valid, then the null check near there has been missing in 4.3?) and the bug does not occur in my environment.

[YeldhamDev]

@TokageItLab I'm able to reproduce it on my end, GNU/Linux. And PR's code fixes it.

[fire]

The logic to check for empty properties to avoid the crash makes sense. Also, the crash has been confirmed. Thirdly, I don't see how it will break the code even if it's wrong, so the risk of merging is low and the benefit of fixing a crash is high.

[TopherBriggs]

Here's the video capture of the crash reported by #98751 replicated on my machine:

Pre-change.mp4

With the change in this PR the crash no longer occurs:

Post-change.mp4

I also added the requested formatting change

[AThousandShips]

Need to confirm that the 4.3 crash is related so unlocking the issue until we can confirm a dedicated 4.3 version fixes this as the code is different

[TokageItLab]

I cannot reproduce the crash in my environment when I perform the same operation as in the video, so it may be an environment-dependent (Linux?) issue.

What I am concerned about is that this fix is not directly related to the animation, but is a check on the processing timing of the inspector plugin.

Considering that the inspector is displayed correctly after the fix, is it possible that the inspector plugin is being called multiple times and somewhere along the way null accesses are occurring? I think we need to take a closer look at why the property is missing there.

Well, as fire's comment says, it should be a safe change, so I think it's okay with merging it to prevent crashes for now.

[akien-mga]

I think #98751 was wrongly flagged as being the issue this solves, though I need to see if I can reproduce it to confirm.

This seems to be solving a very recent regression in master which is tracked in #99249. This one is reproducible for me in latest master but not 4.4-dev4 or 4.3-stable, and fixed by this PR.

[akien-mga]

Confirmed to be fixed a bug introduced by #97415 so it's not fixing #98751.

[Repiteo]

Thanks! Congratulations on your first contribution! 🎉