Fix #118637: crash after editbone duplication in certain case #118676

Philipp Oeser · 2024-02-23T17:14:01+01:00

Philipp Oeser commented

2024-02-23 17:14:01 +01:00

Crash happens in action_group_colors_set_from_posebone /
ANIM_bonecolor_posebone_get on a bPoseChannel without a bone.
If I am not mistaken a new bPoseChannel (e.g. after duplication) will
only get its bone after leaving editmode.

So in a way the situation is similar to 2a8ce1f121

Behavior of animchan_sync_group is not reliable in a way that getting
a bPoseChannel from an bActionGroup will guarantee these are really
corresponding. So usually, if you dulplicate/symmetrize a bone, there
would be no corresponding bActionGroup and nothing would happen
really. But you could for example group fcurves from Bone under a
group called Bone.001 and vice versa. This is totally allowed to do.
In this case, animchan_sync_group is doing nothing totally helpful, so
it could find the "wrong" bPoseChannel. And it could try
action_group_colors_set_from_posebone with that bPoseChannel which
still does not have a bone and then crash.

So now only do this if we have a valid bone.

Crash happens in `action_group_colors_set_from_posebone` / `ANIM_bonecolor_posebone_get` on a `bPoseChannel` without a `bone`. If I am not mistaken a new `bPoseChannel` (e.g. after duplication) will only get its `bone` after leaving editmode. So in a way the situation is similar to 2a8ce1f12162 Behavior of `animchan_sync_group` is not reliable in a way that getting a `bPoseChannel` from an `bActionGroup` will guarantee these are really corresponding. So usually, if you dulplicate/symmetrize a bone, there would be no corresponding `bActionGroup` and nothing would happen really. But you could for example group fcurves from `Bone` under a group called `Bone.001` and vice versa. This is totally allowed to do. In this case, `animchan_sync_group` is doing nothing totally helpful, so it could find the "wrong" `bPoseChannel`. And it could try `action_group_colors_set_from_posebone` with that `bPoseChannel` which still does not have a `bone` and then crash. So now only do this if we have a valid `bone`.

❤️ 1

Philipp Oeser added 1 commit 2024-02-23 17:14:13 +01:00

77495e3870 Fix #118637 : crash after editbone duplication in certain case

Crash happens in `action_group_colors_set_from_posebone` /
`ANIM_bonecolor_posebone_get` on a `bPoseChannel` without a `bone`.
If I am not mistaken a new `bPoseChannel` (e.g. after duplication) will
only get its `bone` after leaving editmode.

So in a way the situation is similar to 2a8ce1f121

Behavior of `animchan_sync_group` is not reliable in a way that getting
a `bPoseChannel` from an `bActionGroup` will guarantee these are really
corresponding. So usually, if you dulplicate/symmetrize a bone, there
would be no corresponding `bActionGroup` and nothing would happen
really. But you could for example group fcurves from `Bone` under a
group called `Bone.001` and vice versa. This is totally allowed to do.
In this case, `animchan_sync_group` is doing nothing totally helpful, so
it could find the "wrong" `bPoseChannel`. And it could try
`action_group_colors_set_from_posebone` with that `bPoseChannel` which
still does not have a `bone` and then crash.

So now only do this if we have a valid `bone`.

Philipp Oeser requested review from Sybren A. Stüvel 2024-02-23 17:14:24 +01:00

Philipp Oeser added this to the Animation & Rigging project 2024-02-23 17:14:30 +01:00

Philipp Oeser referenced this pull request

2024-02-23 17:15:16 +01:00

Crash after symmetrizing/duplicating bone (if an bActionGroup with that new name already exists) #118637

Sybren A. Stüvel added the

Module

Animation & Rigging

label 2024-02-26 12:02:24 +01:00

Sybren A. Stüvel added this to the 4.1 milestone 2024-02-26 12:02:25 +01:00

Sybren A. Stüvel approved these changes 2024-02-26 12:45:56 +01:00

Sybren A. Stüvel left a comment

Thanks for the analysis & the fix!

To slowly migrate our code towards more predictability, I'm trying to get as much of the new code to distinguish between "this nullptr parameter makes sense for this function" from "this really cannot be nullptr for this function to do its job". Here I feel we're in the latter case -- "set something based on this posebone" can't do its job without getting a posebone. In the longer run we could make the parameter a reference instead, to drive the point home that it's not optional.

So if you don't mind applying this diff before landing the PR it's a LGTM!

diff --git a/source/blender/animrig/intern/action.cc b/source/blender/animrig/intern/action.cc
index 75e2cb331ac..9e4de3c55aa 100644
--- a/source/blender/animrig/intern/action.cc
+++ b/source/blender/animrig/intern/action.cc
@@ -87,7 +87,7 @@ FCurve *action_fcurve_ensure(Main *bmain,
       agrp = action_groups_add_new(act, group);
 
       /* Sync bone group colors if applicable. */
-      if (ptr && (ptr->type == &RNA_PoseBone)) {
+      if (ptr && (ptr->type == &RNA_PoseBone) && ptr->data) {
         const bPoseChannel *pchan = static_cast<const bPoseChannel *>(ptr->data);
         action_group_colors_set_from_posebone(agrp, pchan);
       }
diff --git a/source/blender/blenkernel/BKE_action.h b/source/blender/blenkernel/BKE_action.h
index fb8dfea95c0..53fccbe3316 100644
--- a/source/blender/blenkernel/BKE_action.h
+++ b/source/blender/blenkernel/BKE_action.h
@@ -136,6 +136,8 @@ void action_group_colors_set(struct bActionGroup *grp, const struct BoneColor *c
  *
  * If `pchan->color` is set to a non-default color, that is used. Otherwise the
  * armature bone color is used.
+ *
+ * Note that if `pchan->bone` is `nullptr`, this function silently does nothing.
  */
 void action_group_colors_set_from_posebone(bActionGroup *grp, const bPoseChannel *pchan);
 
diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc
index 92cc82c3b60..ba1133108f5 100644
--- a/source/blender/blenkernel/intern/action.cc
+++ b/source/blender/blenkernel/intern/action.cc
@@ -374,8 +374,9 @@ void action_group_colors_sync(bActionGroup *grp, const bActionGroup *ref_grp)
 
 void action_group_colors_set_from_posebone(bActionGroup *grp, const bPoseChannel *pchan)
 {
-  /* pchan->bone is only set after leaving editmode. */
-  if (pchan == nullptr || pchan->bone == nullptr) {
+  BLI_assert_msg(pchan, "cannot 'set action group colors from posebone' without a posebone");
+  if (!pchan->bone) {
+    /* pchan->bone is only set after leaving editmode. */
     return;
   }

PS: the check for pchan->bone I kept inside the function. It would create too tight coupling to move that check out, as then the caller would have to know that the function might actually use that pointer.

Thanks for the analysis & the fix! To slowly migrate our code towards more predictability, I'm trying to get as much of the new code to distinguish between "this `nullptr` parameter makes sense for this function" from "this really cannot be `nullptr` for this function to do its job". Here I feel we're in the latter case -- "set something based on this posebone" can't do its job without getting a posebone. In the longer run we could make the parameter a reference instead, to drive the point home that it's not optional. So if you don't mind applying this diff before landing the PR it's a LGTM! ```diff diff --git a/source/blender/animrig/intern/action.cc b/source/blender/animrig/intern/action.cc index 75e2cb331ac..9e4de3c55aa 100644 --- a/source/blender/animrig/intern/action.cc +++ b/source/blender/animrig/intern/action.cc @@ -87,7 +87,7 @@ FCurve *action_fcurve_ensure(Main *bmain, agrp = action_groups_add_new(act, group); /* Sync bone group colors if applicable. */ - if (ptr && (ptr->type == &RNA_PoseBone)) { + if (ptr && (ptr->type == &RNA_PoseBone) && ptr->data) { const bPoseChannel *pchan = static_cast<const bPoseChannel *>(ptr->data); action_group_colors_set_from_posebone(agrp, pchan); } diff --git a/source/blender/blenkernel/BKE_action.h b/source/blender/blenkernel/BKE_action.h index fb8dfea95c0..53fccbe3316 100644 --- a/source/blender/blenkernel/BKE_action.h +++ b/source/blender/blenkernel/BKE_action.h @@ -136,6 +136,8 @@ void action_group_colors_set(struct bActionGroup *grp, const struct BoneColor *c * * If `pchan->color` is set to a non-default color, that is used. Otherwise the * armature bone color is used. + * + * Note that if `pchan->bone` is `nullptr`, this function silently does nothing. */ void action_group_colors_set_from_posebone(bActionGroup *grp, const bPoseChannel *pchan); diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc index 92cc82c3b60..ba1133108f5 100644 --- a/source/blender/blenkernel/intern/action.cc +++ b/source/blender/blenkernel/intern/action.cc @@ -374,8 +374,9 @@ void action_group_colors_sync(bActionGroup *grp, const bActionGroup *ref_grp) void action_group_colors_set_from_posebone(bActionGroup *grp, const bPoseChannel *pchan) { - /* pchan->bone is only set after leaving editmode. */ - if (pchan == nullptr || pchan->bone == nullptr) { + BLI_assert_msg(pchan, "cannot 'set action group colors from posebone' without a posebone"); + if (!pchan->bone) { + /* pchan->bone is only set after leaving editmode. */ return; } ``` PS: the check for `pchan->bone` I kept inside the function. It would create too tight coupling to move that check out, as then the caller would have to know that the function might actually use that pointer.

👍 1

Philipp Oeser added 1 commit 2024-02-26 13:23:03 +01:00

73b8ea0967 additional polish by @dr.sybren thx!

Philipp Oeser merged commit a6060ea8ee into blender-v4.1-release

2024-02-26 17:41:05 +01:00

Philipp Oeser referenced this issue from a commit

2024-02-26 17:41:05 +01:00

Fix #118637: crash after editbone duplication in certain case

Philipp Oeser deleted branch 118637

2024-02-26 17:41:11 +01:00

Raul Fernandez Hernandez referenced this issue from a commit

2024-03-02 00:30:32 +01:00