Skip to content

Commit 329c8f2

Browse files
authored
Fix panic due to non-total ordering in Area::compare_order() (#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
1 parent 7cb8187 commit 329c8f2

File tree

1 file changed

+51
-5
lines changed

1 file changed

+51
-5
lines changed

crates/egui/src/memory/mod.rs

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,17 +1175,19 @@ impl Areas {
11751175
///
11761176
/// May return [`std::cmp::Ordering::Equal`] if the layers are not in the order list.
11771177
pub(crate) fn compare_order(&self, a: LayerId, b: LayerId) -> std::cmp::Ordering {
1178-
if let (Some(a), Some(b)) = (self.order_map.get(&a), self.order_map.get(&b)) {
1179-
a.cmp(b)
1180-
} else {
1181-
a.order.cmp(&b.order)
1178+
// Sort by layer `order` first and use `order_map` to resolve disputes.
1179+
// If `order_map` only contains one layer ID, then the other one will be
1180+
// lower because `None < Some(x)`.
1181+
match a.order.cmp(&b.order) {
1182+
std::cmp::Ordering::Equal => self.order_map.get(&a).cmp(&self.order_map.get(&b)),
1183+
cmp => cmp,
11821184
}
11831185
}
11841186

11851187
pub(crate) fn set_state(&mut self, layer_id: LayerId, state: area::AreaState) {
11861188
self.visible_areas_current_frame.insert(layer_id);
11871189
self.areas.insert(layer_id.id, state);
1188-
if !self.order.iter().any(|x| *x == layer_id) {
1190+
if !self.order.contains(&layer_id) {
11891191
self.order.push(layer_id);
11901192
}
11911193
}
@@ -1351,3 +1353,47 @@ fn memory_impl_send_sync() {
13511353
fn assert_send_sync<T: Send + Sync>() {}
13521354
assert_send_sync::<Memory>();
13531355
}
1356+
1357+
#[test]
1358+
fn order_map_total_ordering() {
1359+
let mut layers = [
1360+
LayerId::new(Order::Tooltip, Id::new("a")),
1361+
LayerId::new(Order::Background, Id::new("b")),
1362+
LayerId::new(Order::Background, Id::new("c")),
1363+
LayerId::new(Order::Tooltip, Id::new("d")),
1364+
LayerId::new(Order::Background, Id::new("e")),
1365+
LayerId::new(Order::Background, Id::new("f")),
1366+
LayerId::new(Order::Tooltip, Id::new("g")),
1367+
];
1368+
let mut areas = Areas::default();
1369+
1370+
// skip some of the layers
1371+
for &layer in &layers[3..] {
1372+
areas.set_state(layer, crate::AreaState::default());
1373+
}
1374+
areas.end_pass(); // sort layers
1375+
1376+
// Sort layers
1377+
layers.sort_by(|&a, &b| areas.compare_order(a, b));
1378+
1379+
// Assert that `areas.compare_order()` forms a total ordering
1380+
let mut equivalence_classes = vec![0];
1381+
let mut i = 0;
1382+
for l in layers.windows(2) {
1383+
assert!(l[0].order <= l[1].order, "does not follow LayerId.order");
1384+
if areas.compare_order(l[0], l[1]) != std::cmp::Ordering::Equal {
1385+
i += 1;
1386+
}
1387+
equivalence_classes.push(i);
1388+
}
1389+
assert_eq!(layers.len(), equivalence_classes.len());
1390+
for (&l1, c1) in std::iter::zip(&layers, &equivalence_classes) {
1391+
for (&l2, c2) in std::iter::zip(&layers, &equivalence_classes) {
1392+
assert_eq!(
1393+
c1.cmp(c2),
1394+
areas.compare_order(l1, l2),
1395+
"not a total ordering",
1396+
);
1397+
}
1398+
}
1399+
}

0 commit comments

Comments
 (0)