From cd94859a3ef20447674f393d3f169bd9e29023a1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 27 Jul 2023 20:00:25 +0000 Subject: [PATCH] Fix strict-aliasing violation on traits holding inner fields When we map a trait method which returns a reference to a struct, we map it by storing said struct in the trait implementation struct. Then, we create a setter method which ensures the new field is set. Sadly, because the original Rust trait method may take a non-mutable reference to self, we have to update the field without a mutable reference to the trait implementation struct. Previously we did this by simply unsafe-casting a pointer to mutable, which violates the aliasing rules in Rust. In a recent rustc (at least on macOS), this broke. Here, we convert the stored struct to wrap it in an `UnsafeCell`, which fixes the issue. --- c-bindings-gen/src/main.rs | 20 ++++++++++---------- genbindings.sh | 6 ++++++ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/c-bindings-gen/src/main.rs b/c-bindings-gen/src/main.rs index 7106400..1b54d16 100644 --- a/c-bindings-gen/src/main.rs +++ b/c-bindings-gen/src/main.rs @@ -351,10 +351,10 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty // the Rust type and a flag to indicate whether deallocation needs to // happen) as well as provide an Option<>al function pointer which is // called when the trait method is called which allows updating on the fly. - write!(w, "\tpub {}: ", m.sig.ident).unwrap(); - generated_fields.push((format!("{}", m.sig.ident), None, None)); + write!(w, "\tpub {}: core::cell::UnsafeCell<", m.sig.ident).unwrap(); + generated_fields.push((format!("{}", m.sig.ident), Some(("Clone::clone(unsafe { &*core::cell::UnsafeCell::get(".to_owned(), ")}).into()")), None)); types.write_c_type(w, &*r.elem, Some(&meth_gen_types), false); - writeln!(w, ",").unwrap(); + writeln!(w, ">,").unwrap(); writeln!(w, "\t/// Fill in the {} field as a reference to it will be given to Rust after this returns", m.sig.ident).unwrap(); writeln!(w, "\t/// Note that this takes a pointer to this object, not the this_ptr like other methods do").unwrap(); writeln!(w, "\t/// This function pointer may be NULL if {} is filled in when this object is created and never needs updating.", m.sig.ident).unwrap(); @@ -430,7 +430,7 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty let is_clonable = types.is_clonable(s); writeln!(w, "\tpub {}: crate::{},", i, s).unwrap(); (format!("{}", i), if !is_clonable { - Some(format!("crate::{}_clone_fields", s)) + Some((format!("crate::{}_clone_fields(", s), ")")) } else { None }, None) }); } @@ -513,7 +513,7 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty writeln!(w, "\t\t\t(f)(&self{});", $impl_accessor).unwrap(); write!(w, "\t\t}}\n\t\t").unwrap(); $type_resolver.write_from_c_conversion_to_ref_prefix(w, &*r.elem, Some(&meth_gen_types)); - write!(w, "self{}.{}", $impl_accessor, m.sig.ident).unwrap(); + write!(w, "unsafe {{ &*self{}.{}.get() }}", $impl_accessor, m.sig.ident).unwrap(); $type_resolver.write_from_c_conversion_to_ref_suffix(w, &*r.elem, Some(&meth_gen_types)); writeln!(w, "\n\t}}").unwrap(); continue; @@ -557,9 +557,9 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty writeln!(w, "\t{} {{", trait_name).unwrap(); writeln!(w, "\t\tthis_arg: orig.this_arg,").unwrap(); for (field, clone_fn, _) in generated_fields.iter() { - if let Some(f) = clone_fn { + if let Some((pfx, sfx)) = clone_fn { // If the field isn't clonable, blindly assume its a trait and hope for the best. - writeln!(w, "\t\t{}: {}(&orig.{}),", field, f, field).unwrap(); + writeln!(w, "\t\t{}: {}&orig.{}{},", field, pfx, field, sfx).unwrap(); } else { writeln!(w, "\t\t{}: Clone::clone(&orig.{}),", field, field).unwrap(); } @@ -1046,7 +1046,7 @@ fn writeln_impl(w: &mut W, w_uses: &mut HashSet(w: &mut W, w_uses: &mut HashSet/dev/null 2>&1 if is_gnu_sed; then sed -i 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h + # UnsafeCell is `repr(transparent)` so should be ignored here + sed -i 's/LDKUnsafeCell<\(.*\)> /\1 /g' include/lightning.h + # stdlib.h doesn't exist in clang's wasm sysroot, and cbindgen # doesn't actually use it anyway, so drop the import. sed -i 's/#include /#include "ldk_rust_types.h"/g' include/lightning.h @@ -238,6 +241,9 @@ else # OSX sed is for some reason not compatible with GNU sed sed -i '' 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h + # UnsafeCell is `repr(transparent)` so should be ignored by cbindgen + sed -i '' 's/LDKUnsafeCell<\(.*\)> /\1 /g' include/lightning.h + # stdlib.h doesn't exist in clang's wasm sysroot, and cbindgen # doesn't actually use it anyway, so drop the import. sed -i '' 's/#include /#include "ldk_rust_types.h"/g' include/lightning.h -- 2.39.5