[Bf-blender-cvs] [d3949a4fdb1] master: Fix GHOST/Wayland thread-unsafe key-repeat timer checks
Campbell Barton
noreply at git.blender.org
Mon Feb 6 02:51:09 CET 2023
Commit: d3949a4fdb1c4ebc67e6ebd3af5792a3c2a51044
Author: Campbell Barton
Date: Mon Feb 6 11:09:29 2023 +1100
Branches: master
https://developer.blender.org/rBd3949a4fdb1c4ebc67e6ebd3af5792a3c2a51044
Fix GHOST/Wayland thread-unsafe key-repeat timer checks
Resolve a thread safety issue reported by valgrind's helgrind checker,
although I wasn't able to redo the error in practice.
NULL check on the key-repeat timer also needs to lock, otherwise it's
possible the timer is set in another thread before the lock is acquired.
Now all key-repeat timer access which may run from a thread
locks the timer mutex before any checks or timer manipulation.
===================================================================
M intern/ghost/intern/GHOST_SystemWayland.cpp
===================================================================
diff --git a/intern/ghost/intern/GHOST_SystemWayland.cpp b/intern/ghost/intern/GHOST_SystemWayland.cpp
index 496179fc826..8587438a34b 100644
--- a/intern/ghost/intern/GHOST_SystemWayland.cpp
+++ b/intern/ghost/intern/GHOST_SystemWayland.cpp
@@ -768,7 +768,12 @@ struct GWL_Seat {
int32_t rate = 0;
/** Time (milliseconds) after which to start repeating keys. */
int32_t delay = 0;
- /** Timer for key repeats. */
+ /**
+ * Timer for key repeats.
+ *
+ * \note For as long as #USE_EVENT_BACKGROUND_THREAD is defined, any access to this
+ * (including null checks, must lock `timer_mutex` first.
+ */
GHOST_ITimerTask *timer = nullptr;
} key_repeat;
@@ -832,6 +837,30 @@ static bool gwl_seat_key_depressed_suppress_warning(const GWL_Seat *seat)
return suppress_warning;
}
+/**
+ * \note Caller must lock `timer_mutex`.
+ */
+static void gwl_seat_key_repeat_timer_add(GWL_Seat *seat,
+ GHOST_TimerProcPtr key_repeat_fn,
+ GHOST_TUserDataPtr payload,
+ const bool use_delay)
+{
+ GHOST_SystemWayland *system = seat->system;
+ const uint64_t time_step = 1000 / seat->key_repeat.rate;
+ const uint64_t time_start = use_delay ? seat->key_repeat.delay : time_step;
+ seat->key_repeat.timer = system->installTimer(time_start, time_step, key_repeat_fn, payload);
+}
+
+/**
+ * \note The caller must lock `timer_mutex`.
+ */
+static void gwl_seat_key_repeat_timer_remove(GWL_Seat *seat)
+{
+ GHOST_SystemWayland *system = seat->system;
+ system->removeTimer(seat->key_repeat.timer);
+ seat->key_repeat.timer = nullptr;
+}
+
/** \} */
/* -------------------------------------------------------------------- */
@@ -3718,9 +3747,14 @@ static void keyboard_handle_leave(void *data,
GWL_Seat *seat = static_cast<GWL_Seat *>(data);
seat->keyboard.wl_surface_window = nullptr;
- /* Losing focus must stop repeating text. */
- if (seat->key_repeat.timer) {
- keyboard_handle_key_repeat_cancel(seat);
+ {
+#ifdef USE_EVENT_BACKGROUND_THREAD
+ std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
+#endif
+ /* Losing focus must stop repeating text. */
+ if (seat->key_repeat.timer) {
+ keyboard_handle_key_repeat_cancel(seat);
+ }
}
#ifdef USE_GNOME_KEYBOARD_SUPPRESS_WARNING
@@ -3780,36 +3814,32 @@ static xkb_keysym_t xkb_state_key_get_one_sym_without_modifiers(
return sym;
}
+/**
+ * \note Caller must lock `timer_mutex`.
+ */
static void keyboard_handle_key_repeat_cancel(GWL_Seat *seat)
{
-#ifdef USE_EVENT_BACKGROUND_THREAD
- std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
-#endif
GHOST_ASSERT(seat->key_repeat.timer != nullptr, "Caller much check for timer");
delete static_cast<GWL_KeyRepeatPlayload *>(seat->key_repeat.timer->getUserData());
- seat->system->removeTimer(seat->key_repeat.timer);
- seat->key_repeat.timer = nullptr;
+
+ gwl_seat_key_repeat_timer_remove(seat);
}
/**
* Restart the key-repeat timer.
* \param use_delay: When false, use the interval
* (prevents pause when the setting changes while the key is held).
+ *
+ * \note Caller must lock `timer_mutex`.
*/
static void keyboard_handle_key_repeat_reset(GWL_Seat *seat, const bool use_delay)
{
-#ifdef USE_EVENT_BACKGROUND_THREAD
- std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
-#endif
GHOST_ASSERT(seat->key_repeat.timer != nullptr, "Caller much check for timer");
- GHOST_SystemWayland *system = seat->system;
- GHOST_ITimerTask *timer = seat->key_repeat.timer;
- GHOST_TimerProcPtr key_repeat_fn = timer->getTimerProc();
+ GHOST_TimerProcPtr key_repeat_fn = seat->key_repeat.timer->getTimerProc();
GHOST_TUserDataPtr payload = seat->key_repeat.timer->getUserData();
- seat->system->removeTimer(seat->key_repeat.timer);
- const uint64_t time_step = 1000 / seat->key_repeat.rate;
- const uint64_t time_start = use_delay ? seat->key_repeat.delay : time_step;
- seat->key_repeat.timer = system->installTimer(time_start, time_step, key_repeat_fn, payload);
+
+ gwl_seat_key_repeat_timer_remove(seat);
+ gwl_seat_key_repeat_timer_add(seat, key_repeat_fn, payload, use_delay);
}
static void keyboard_handle_key(void *data,
@@ -3848,6 +3878,11 @@ static void keyboard_handle_key(void *data,
break;
}
+#ifdef USE_EVENT_BACKGROUND_THREAD
+ /* Any access to `seat->key_repeat.timer` must lock. */
+ std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
+#endif
+
struct GWL_KeyRepeatPlayload *key_repeat_payload = nullptr;
/* Delete previous timer. */
@@ -3886,23 +3921,14 @@ static void keyboard_handle_key(void *data,
break;
}
case RESET: {
-#ifdef USE_EVENT_BACKGROUND_THREAD
- std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
-#endif
/* The payload will be added again. */
- seat->system->removeTimer(seat->key_repeat.timer);
- seat->key_repeat.timer = nullptr;
+ gwl_seat_key_repeat_timer_remove(seat);
break;
}
case CANCEL: {
-#ifdef USE_EVENT_BACKGROUND_THREAD
- std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
-#endif
delete key_repeat_payload;
key_repeat_payload = nullptr;
-
- seat->system->removeTimer(seat->key_repeat.timer);
- seat->key_repeat.timer = nullptr;
+ gwl_seat_key_repeat_timer_remove(seat);
break;
}
}
@@ -3956,8 +3982,8 @@ static void keyboard_handle_key(void *data,
utf8_buf));
}
};
- seat->key_repeat.timer = seat->system->installTimer(
- seat->key_repeat.delay, 1000 / seat->key_repeat.rate, key_repeat_fn, key_repeat_payload);
+
+ gwl_seat_key_repeat_timer_add(seat, key_repeat_fn, key_repeat_payload, true);
}
}
@@ -3982,8 +4008,13 @@ static void keyboard_handle_modifiers(void *data,
/* A modifier changed so reset the timer,
* see comment in #keyboard_handle_key regarding this behavior. */
- if (seat->key_repeat.timer) {
- keyboard_handle_key_repeat_reset(seat, true);
+ {
+#ifdef USE_EVENT_BACKGROUND_THREAD
+ std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
+#endif
+ if (seat->key_repeat.timer) {
+ keyboard_handle_key_repeat_reset(seat, true);
+ }
}
#ifdef USE_GNOME_KEYBOARD_SUPPRESS_WARNING
@@ -4002,9 +4033,14 @@ static void keyboard_repeat_handle_info(void *data,
seat->key_repeat.rate = rate;
seat->key_repeat.delay = delay;
- /* Unlikely possible this setting changes while repeating. */
- if (seat->key_repeat.timer) {
- keyboard_handle_key_repeat_reset(seat, false);
+ {
+#ifdef USE_EVENT_BACKGROUND_THREAD
+ std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
+#endif
+ /* Unlikely possible this setting changes while repeating. */
+ if (seat->key_repeat.timer) {
+ keyboard_handle_key_repeat_reset(seat, false);
+ }
}
}
@@ -4275,8 +4311,14 @@ static void gwl_seat_capability_keyboard_disable(GWL_Seat *seat)
if (!seat->wl_keyboard) {
return;
}
- if (seat->key_repeat.timer) {
- keyboard_handle_key_repeat_cancel(seat);
+
+ {
+#ifdef USE_EVENT_BACKGROUND_THREAD
+ std::lock_guard lock_timer_guard{*seat->system->timer_mutex};
+#endif
+ if (seat->key_repeat.timer) {
+ keyboard_handle_key_repeat_cancel(seat);
+ }
}
wl_keyboard_destroy(seat->wl_keyboard);
seat->wl_keyboard = nullptr;
More information about the Bf-blender-cvs
mailing list