Skip to content

Commit

Permalink
Fix panic due to non-total ordering in Area::compare_order() (emilk…
Browse files Browse the repository at this point in the history
…#5569)

[Area::compare_order()](https://github.com/emilk/egui/blob/ee4ab08c8a208f4044a8c571326c9414e7a1c8a6/crates/egui/src/memory/mod.rs#L1174-L1183)
is not a total ordering. If three layers A, B, and C have the same
`order` field but only A and B are present in `order_map`, then `A==C`
and `B==C` but `A!=C`. This can cause a panic in the stdlib sort
function, and does in [my
app](https://github.com/HactarCE/Hyperspeedcube/tree/v2.0) although it's
very difficult to reproduce.

* [x] I have self-reviewed this PR and run `./scripts/check.sh`
* [x] I have followed the instructions in the PR template
  • Loading branch information
HactarCE authored Jan 7, 2025
1 parent 7cb8187 commit 329c8f2
Showing 1 changed file with 51 additions and 5 deletions.
56 changes: 51 additions & 5 deletions crates/egui/src/memory/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1175,17 +1175,19 @@ impl Areas {
///
/// May return [`std::cmp::Ordering::Equal`] if the layers are not in the order list.
pub(crate) fn compare_order(&self, a: LayerId, b: LayerId) -> std::cmp::Ordering {
if let (Some(a), Some(b)) = (self.order_map.get(&a), self.order_map.get(&b)) {
a.cmp(b)
} else {
a.order.cmp(&b.order)
// Sort by layer `order` first and use `order_map` to resolve disputes.
// If `order_map` only contains one layer ID, then the other one will be
// lower because `None < Some(x)`.
match a.order.cmp(&b.order) {
std::cmp::Ordering::Equal => self.order_map.get(&a).cmp(&self.order_map.get(&b)),
cmp => cmp,
}
}

pub(crate) fn set_state(&mut self, layer_id: LayerId, state: area::AreaState) {
self.visible_areas_current_frame.insert(layer_id);
self.areas.insert(layer_id.id, state);
if !self.order.iter().any(|x| *x == layer_id) {
if !self.order.contains(&layer_id) {
self.order.push(layer_id);
}
}
Expand Down Expand Up @@ -1351,3 +1353,47 @@ fn memory_impl_send_sync() {
fn assert_send_sync<T: Send + Sync>() {}
assert_send_sync::<Memory>();
}

#[test]
fn order_map_total_ordering() {
let mut layers = [
LayerId::new(Order::Tooltip, Id::new("a")),
LayerId::new(Order::Background, Id::new("b")),
LayerId::new(Order::Background, Id::new("c")),
LayerId::new(Order::Tooltip, Id::new("d")),
LayerId::new(Order::Background, Id::new("e")),
LayerId::new(Order::Background, Id::new("f")),
LayerId::new(Order::Tooltip, Id::new("g")),
];
let mut areas = Areas::default();

// skip some of the layers
for &layer in &layers[3..] {
areas.set_state(layer, crate::AreaState::default());
}
areas.end_pass(); // sort layers

// Sort layers
layers.sort_by(|&a, &b| areas.compare_order(a, b));

// Assert that `areas.compare_order()` forms a total ordering
let mut equivalence_classes = vec![0];
let mut i = 0;
for l in layers.windows(2) {
assert!(l[0].order <= l[1].order, "does not follow LayerId.order");
if areas.compare_order(l[0], l[1]) != std::cmp::Ordering::Equal {
i += 1;
}
equivalence_classes.push(i);
}
assert_eq!(layers.len(), equivalence_classes.len());
for (&l1, c1) in std::iter::zip(&layers, &equivalence_classes) {
for (&l2, c2) in std::iter::zip(&layers, &equivalence_classes) {
assert_eq!(
c1.cmp(c2),
areas.compare_order(l1, l2),
"not a total ordering",
);
}
}
}

0 comments on commit 329c8f2

Please sign in to comment.