From 8bb381d50bf181c4ca34ae1d92176e2672fd2b6e Mon Sep 17 00:00:00 2001 From: Zachary Kohnen Date: Sun, 20 Mar 2022 20:30:38 +0100 Subject: [PATCH] Fix code that could lead to a possible deadlock. (#1380) * Fix code that could lead to a possible deadlock. Drop implementations are not called until the end of a statement. The statement changed in this commit therefore took 4 read locks on a RwLock which can lead to problems if a write lock is requested between any of these read locks. The code looks like it would only hold one lock at a time but it does not drop any of the locks until after the arithmatic operations complete, which leads to this situation. See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=996079046184329f3a9df1cd19c87da8 to see this in action. The fix is to just take one lock and share it between the three calls to num_presses, letting it drop at the end of the scope. * Fix code that may cause a deadlock in `MenuRoot::stationary_interaction` The issue here is related to that in 9673b8f2a08302c10ffcfd063f2dbdec4301d3e2 in that the lock is not dropped when it is expected. Since the `RwLockReadGuard` produced by `ctx.input()` has a reference taken from it (one into `pointer`), the lock cannot be dropped until that reference is no longre valid, which is the end of the scope (in this case this function). The following `ctx.input()` then attempts to aquire a second lock on the `RwLock`, creating the same situation that was found in the referenced commit. This has been resolved by holding one lock on the input for the whole function. * Reference this PR from comments in the code for future maintainers * Add the change to the changelog * Use full link to PR * Use full link to PR Co-authored-by: Emil Ernerfeldt --- CHANGELOG.md | 2 +- egui/src/menu.rs | 12 ++++++------ egui/src/widgets/slider.rs | 12 ++++++++---- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6678da2cb1b..3612db7098a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ NOTE: [`epaint`](epaint/CHANGELOG.md), [`eframe`](eframe/CHANGELOG.md), [`egui_w ### Fixed 🐛 * Fixed ComboBoxes always being rendered left-aligned ([#1304](https://github.com/emilk/egui/pull/1304)). - +* Fixed ui code that could lead to a deadlock ([#1380](https://github.com/emilk/egui/pull/1380)) ## 0.17.0 - 2022-02-22 - Improved font selection and image handling diff --git a/egui/src/menu.rs b/egui/src/menu.rs index 46146821ab2..1906150d08f 100644 --- a/egui/src/menu.rs +++ b/egui/src/menu.rs @@ -277,10 +277,10 @@ impl MenuRoot { root: &mut MenuRootManager, id: Id, ) -> MenuResponse { - let pointer = &response.ctx.input().pointer; - if (response.clicked() && root.is_menu_open(id)) - || response.ctx.input().key_pressed(Key::Escape) - { + // Lock the input once for the whole function call (see https://github.com/emilk/egui/pull/1380). + let input = response.ctx.input(); + + if (response.clicked() && root.is_menu_open(id)) || input.key_pressed(Key::Escape) { // menu open and button clicked or esc pressed return MenuResponse::Close; } else if (response.clicked() && !root.is_menu_open(id)) @@ -290,8 +290,8 @@ impl MenuRoot { // or button hovered while other menu is open let pos = response.rect.left_bottom(); return MenuResponse::Create(pos, id); - } else if pointer.any_pressed() && pointer.primary_down() { - if let Some(pos) = pointer.interact_pos() { + } else if input.pointer.any_pressed() && input.pointer.primary_down() { + if let Some(pos) = input.pointer.interact_pos() { if let Some(root) = root.inner.as_mut() { if root.id == id { // pressed somewhere while this menu is open diff --git a/egui/src/widgets/slider.rs b/egui/src/widgets/slider.rs index 23480abeece..c24a5fb9cc3 100644 --- a/egui/src/widgets/slider.rs +++ b/egui/src/widgets/slider.rs @@ -455,10 +455,14 @@ impl<'a> Slider<'a> { fn value_ui(&mut self, ui: &mut Ui, position_range: RangeInclusive) -> Response { // If `DragValue` is controlled from the keyboard and `step` is defined, set speed to `step` - let change = ui.input().num_presses(Key::ArrowUp) as i32 - + ui.input().num_presses(Key::ArrowRight) as i32 - - ui.input().num_presses(Key::ArrowDown) as i32 - - ui.input().num_presses(Key::ArrowLeft) as i32; + let change = { + // Hold one lock rather than 4 (see https://github.com/emilk/egui/pull/1380). + let input = ui.input(); + + input.num_presses(Key::ArrowUp) as i32 + input.num_presses(Key::ArrowRight) as i32 + - input.num_presses(Key::ArrowDown) as i32 + - input.num_presses(Key::ArrowLeft) as i32 + }; let speed = match self.step { Some(step) if change != 0 => step, _ => self.current_gradient(&position_range),