From a6dffb6ef91bcf8995ef44261796463bf13723f4 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Wed, 8 Feb 2023 14:25:25 +0200 Subject: [PATCH] fix: stop using Arc::ptr_eq with dyn Trait (#3558) This changes the way we compare `Arc` in Timeline's `LayerMap` not to use `Arc::ptr_eq` which has been witnessed in development of #3557 to yield wrong results. It gives wrong results because it compares fat pointers, which are `(object, vtable)` tuples for `dyn Trait` and there are no guarantees that the `vtable`s are unique. As in there were multiple vtables for `RemoteLayer` which is why the comparison failed in #3557. This is a known issue in rust, clippy warns against it and rust std might be moving to the solution which has been reproduced on this PR: compare only object pointers by "casting out" the vtable pointer. --- pageserver/src/tenant/layer_map.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 59a358a355..9d8c825220 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -733,14 +733,25 @@ where #[inline(always)] fn compare_arced_layers(left: &Arc, right: &Arc) -> bool { - // FIXME: ptr_eq might fail to return true for 'dyn' references because of multiple vtables - // can be created in compilation. Clippy complains about this. In practice it seems to - // work. + // "dyn Trait" objects are "fat pointers" in that they have two components: + // - pointer to the object + // - pointer to the vtable // - // In future rust versions this might become Arc::as_ptr(left) as *const () == - // Arc::as_ptr(right) as *const (), we could change to that before. - #[allow(clippy::vtable_address_comparisons)] - Arc::ptr_eq(left, right) + // rust does not provide a guarantee that these vtables are unique, but however + // `Arc::ptr_eq` as of writing (at least up to 1.67) uses a comparison where both the + // pointer and the vtable need to be equal. + // + // See: https://github.com/rust-lang/rust/issues/103763 + // + // A future version of rust will most likely use this form below, where we cast each + // pointer into a pointer to unit, which drops the inaccessible vtable pointer, making it + // not affect the comparison. + // + // See: https://github.com/rust-lang/rust/pull/106450 + let left = Arc::as_ptr(left) as *const (); + let right = Arc::as_ptr(right) as *const (); + + left == right } }