Skip to content

Commit cd94859

Browse files
committed
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.
1 parent 316ba2b commit cd94859

File tree

2 files changed

+16
-10
lines changed

2 files changed

+16
-10
lines changed

c-bindings-gen/src/main.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -351,10 +351,10 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty
351351
// the Rust type and a flag to indicate whether deallocation needs to
352352
// happen) as well as provide an Option<>al function pointer which is
353353
// called when the trait method is called which allows updating on the fly.
354-
write!(w, "\tpub {}: ", m.sig.ident).unwrap();
355-
generated_fields.push((format!("{}", m.sig.ident), None, None));
354+
write!(w, "\tpub {}: core::cell::UnsafeCell<", m.sig.ident).unwrap();
355+
generated_fields.push((format!("{}", m.sig.ident), Some(("Clone::clone(unsafe { &*core::cell::UnsafeCell::get(".to_owned(), ")}).into()")), None));
356356
types.write_c_type(w, &*r.elem, Some(&meth_gen_types), false);
357-
writeln!(w, ",").unwrap();
357+
writeln!(w, ">,").unwrap();
358358
writeln!(w, "\t/// Fill in the {} field as a reference to it will be given to Rust after this returns", m.sig.ident).unwrap();
359359
writeln!(w, "\t/// Note that this takes a pointer to this object, not the this_ptr like other methods do").unwrap();
360360
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
430430
let is_clonable = types.is_clonable(s);
431431
writeln!(w, "\tpub {}: crate::{},", i, s).unwrap();
432432
(format!("{}", i), if !is_clonable {
433-
Some(format!("crate::{}_clone_fields", s))
433+
Some((format!("crate::{}_clone_fields(", s), ")"))
434434
} else { None }, None)
435435
});
436436
}
@@ -513,7 +513,7 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty
513513
writeln!(w, "\t\t\t(f)(&self{});", $impl_accessor).unwrap();
514514
write!(w, "\t\t}}\n\t\t").unwrap();
515515
$type_resolver.write_from_c_conversion_to_ref_prefix(w, &*r.elem, Some(&meth_gen_types));
516-
write!(w, "self{}.{}", $impl_accessor, m.sig.ident).unwrap();
516+
write!(w, "unsafe {{ &*self{}.{}.get() }}", $impl_accessor, m.sig.ident).unwrap();
517517
$type_resolver.write_from_c_conversion_to_ref_suffix(w, &*r.elem, Some(&meth_gen_types));
518518
writeln!(w, "\n\t}}").unwrap();
519519
continue;
@@ -557,9 +557,9 @@ fn writeln_trait<'a, 'b, W: std::io::Write>(w: &mut W, t: &'a syn::ItemTrait, ty
557557
writeln!(w, "\t{} {{", trait_name).unwrap();
558558
writeln!(w, "\t\tthis_arg: orig.this_arg,").unwrap();
559559
for (field, clone_fn, _) in generated_fields.iter() {
560-
if let Some(f) = clone_fn {
560+
if let Some((pfx, sfx)) = clone_fn {
561561
// If the field isn't clonable, blindly assume its a trait and hope for the best.
562-
writeln!(w, "\t\t{}: {}(&orig.{}),", field, f, field).unwrap();
562+
writeln!(w, "\t\t{}: {}&orig.{}{},", field, pfx, field, sfx).unwrap();
563563
} else {
564564
writeln!(w, "\t\t{}: Clone::clone(&orig.{}),", field, field).unwrap();
565565
}
@@ -1046,7 +1046,7 @@ fn writeln_impl<W: std::io::Write>(w: &mut W, w_uses: &mut HashSet<String, NonRa
10461046
if let syn::Type::Reference(r) = &**rtype {
10471047
write!(w, "\n\t\t{}{}: ", $indent, $m.sig.ident).unwrap();
10481048
types.write_empty_rust_val(Some(&gen_types), w, &*r.elem);
1049-
writeln!(w, ",\n{}\t\tset_{}: Some({}_{}_set_{}),", $indent, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
1049+
writeln!(w, ".into(),\n{}\t\tset_{}: Some({}_{}_set_{}),", $indent, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
10501050
printed = true;
10511051
}
10521052
}
@@ -1198,9 +1198,9 @@ fn writeln_impl<W: std::io::Write>(w: &mut W, w_uses: &mut HashSet<String, NonRa
11981198
writeln!(w, "\t// This is a bit race-y in the general case, but for our specific use-cases today, we're safe").unwrap();
11991199
writeln!(w, "\t// Specifically, we must ensure that the first time we're called it can never be in parallel").unwrap();
12001200
write!(w, "\tif ").unwrap();
1201-
$types.write_empty_rust_val_check(Some(&meth_gen_types), w, &*r.elem, &format!("trait_self_arg.{}", $m.sig.ident));
1201+
$types.write_empty_rust_val_check(Some(&meth_gen_types), w, &*r.elem, &format!("unsafe {{ &*trait_self_arg.{}.get() }}", $m.sig.ident));
12021202
writeln!(w, " {{").unwrap();
1203-
writeln!(w, "\t\tunsafe {{ &mut *(trait_self_arg as *const {} as *mut {}) }}.{} = {}_{}_{}(trait_self_arg.this_arg);", $trait.ident, $trait.ident, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
1203+
writeln!(w, "\t\t*unsafe {{ &mut *(&*(trait_self_arg as *const {})).{}.get() }} = {}_{}_{}(trait_self_arg.this_arg).into();", $trait.ident, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
12041204
writeln!(w, "\t}}").unwrap();
12051205
writeln!(w, "}}").unwrap();
12061206
}

genbindings.sh

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,13 +231,19 @@ cbindgen -v --config cbindgen.toml -o include/lightning.h >/dev/null 2>&1
231231
if is_gnu_sed; then
232232
sed -i 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h
233233

234+
# UnsafeCell is `repr(transparent)` so should be ignored here
235+
sed -i 's/LDKUnsafeCell<\(.*\)> /\1 /g' include/lightning.h
236+
234237
# stdlib.h doesn't exist in clang's wasm sysroot, and cbindgen
235238
# doesn't actually use it anyway, so drop the import.
236239
sed -i 's/#include <stdlib.h>/#include "ldk_rust_types.h"/g' include/lightning.h
237240
else
238241
# OSX sed is for some reason not compatible with GNU sed
239242
sed -i '' 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h
240243

244+
# UnsafeCell is `repr(transparent)` so should be ignored by cbindgen
245+
sed -i '' 's/LDKUnsafeCell<\(.*\)> /\1 /g' include/lightning.h
246+
241247
# stdlib.h doesn't exist in clang's wasm sysroot, and cbindgen
242248
# doesn't actually use it anyway, so drop the import.
243249
sed -i '' 's/#include <stdlib.h>/#include "ldk_rust_types.h"/g' include/lightning.h

0 commit comments

Comments
 (0)