From 73769a030d127107be84e052b4afa3cbc62e77ee Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Mon, 12 Feb 2024 00:46:04 -0800 Subject: [PATCH] Fix msan issue in Map PiperOrigin-RevId: 606164792 --- rust/test/shared/accessors_map_test.rs | 3 +-- rust/upb.rs | 22 ++++++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/rust/test/shared/accessors_map_test.rs b/rust/test/shared/accessors_map_test.rs index 4988cde1d0..75730b67b8 100644 --- a/rust/test/shared/accessors_map_test.rs +++ b/rust/test/shared/accessors_map_test.rs @@ -292,7 +292,6 @@ generate_map_with_msg_values_tests!( (fixed64, 1u64, 2u64), (sfixed32, 1, 2), (sfixed64, 1, 2), - // TODO - b/324468833: fix msan failure - // (bool, true, false), + (bool, true, false), (string, "foo", "bar"), ); diff --git a/rust/upb.rs b/rust/upb.rs index 8aa9d67647..e5f5d3880f 100644 --- a/rust/upb.rs +++ b/rust/upb.rs @@ -684,12 +684,26 @@ where { // TODO: Consider creating a static empty map in C. - // Use `i32` for a shared empty map for all map types. - static EMPTY_MAP_VIEW: OnceLock> = OnceLock::new(); + // Use `` for a shared empty map for all map types. + // + // This relies on an implicit contract with UPB that it is OK to use an empty + // Map as an empty map of all other types. The only const + // function on `upb_Map` that will care about the size of key or value is + // `get()` where it will hash the appropriate number of bytes of the + // provided `upb_MessageValue`, and that bool being the smallest type in the + // union means it will happen to work for all possible key types. + // + // If we used a larger key, then UPB would hash more bytes of the key than Rust + // initialized. + static EMPTY_MAP_VIEW: OnceLock> = OnceLock::new(); // SAFETY: - // - Because the map is never mutated, the map type is unused and therefore - // valid for `T`. + // - The map is empty and never mutated. + // - The value type is never used. + // - The size of the key type is used when `get()` computes the hash of the key. + // The map is empty, therefore it doesn't matter what hash is computed, but we + // have to use `bool` type as the smallest key possible (otherwise UPB would + // read more bytes than Rust allocated). // - The view is leaked for `'static`. unsafe { MapView::from_raw(