Crash fix: macOS (Silicon) fix crash on joypad removal #55034
Conversation
akien-mga
added
bug
cherrypick:3.3
cherrypick:3.x
|
Looks good, but the two commits are directly related and should be squashed together to be a self-contained fix. See PR workflow. |
|
Thank you for the link explaining that @akien-mga ! I've squashed the commits together now. |
There was a problem hiding this comment.
Change is fine, but need a style fix:
diff --git a/platform/osx/joypad_osx.cpp b/platform/osx/joypad_osx.cpp
index 0b511e3..e4c4ec0 100644
--- a/platform/osx/joypad_osx.cpp
+++ b/platform/osx/joypad_osx.cpp
@@ -349,7 +349,7 @@ bool JoypadOSX::configure_joypad(IOHIDDeviceRef p_device_ref, joypad *p_joy) {
{ \
if (ret != FF_OK) { \
FFReleaseDevice(ff_device); \
- ff_device = 0; \
+ ff_device = 0; \
return false; \
} \
}Also, since it's reference, it probably should be nullptr not 0.
|
Fixed style, and also changed to nullptr. |
platform/osx/joypad_osx.cpp
Outdated
| @@ -55,9 +55,10 @@ void joypad::free() { | |||
| if (device_ref) { | |||
| IOHIDDeviceUnscheduleFromRunLoop(device_ref, CFRunLoopGetCurrent(), GODOT_JOY_LOOP_RUN_MODE); | |||
| } | |||
| if (ff_device) { | |||
| if (ff_device != nullptr) { | |||
There was a problem hiding this comment.
Pointers evaluate well as booleans so we usually use the shorter if (pointer) style instead of making the != nullptr part explicit.
There was a problem hiding this comment.
I was following the style on line 247. I can change both though?
There was a problem hiding this comment.
Yeah it's not very important, but for consistency's sake I guess you can change the other two occurrences on lines 247 and 607.
platform/osx/joypad_osx.h
Outdated
| @@ -68,7 +68,7 @@ struct joypad { | |||
|
|
|||
| io_service_t ffservice = 0; /* Interface for force feedback, 0 = no ff */ | |||
| FFCONSTANTFORCE ff_constant_force; | |||
| FFDeviceObjectReference ff_device; | |||
| FFDeviceObjectReference ff_device = nullptr; | |||
| FFEffectObjectReference ff_object; | |||
There was a problem hiding this comment.
I guess this could be initialized to nullptr too just to be safe.
|
FYI, the email used to author your commit doesn't seem to be linked to your GitHub account (see the email in https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/55034.patch ), which is why the commit is shown as authored and committed by two different identities: It's not a problem per se, but if you want to claim the commit as a contribution for your @plucky-git account, you can add this email as secondary email in your GitHub settings, or edit the commit to change its Git author details. |
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
|
Cherry-picked for 3.5. |
akien-mga
removed
the
cherrypick:3.x
|
Cherry-picked for 3.4.1. |
|
Cherry-picked for 3.3.5. |
ff_device is initialised in config_force_feedback. So in line 250, if FFIsForceFeedback(ioservice) != FF_OK then config_force_feedback isn't called, and ff_device is left uninitialised. This then causes a crash on line 60 when the controller is removed and ff_device is incorrectly freed.
This was noticed when a Dualshock 4 or Dualsense (bluetooth) was left to disconnect, or force disconnected while the build is running with the following stack trace:
This fix ensures the ff_device variable is default intialised to 0, and when released set back to 0.