Merge pull request #111 from TheBlueMatt/2023-07-0.0.115-aliasing-fix

[0.0.115] Fix strict-aliasing violation on traits holding inner fields

Author: TheBlueMatt

.github/workflows/build.yml

@@ -40,6 +40,11 @@ jobs:
           git checkout 0.0.115-bindings
       - name: Fix Github Actions to not be broken
         run: git config --global --add safe.directory /__w/ldk-c-bindings/ldk-c-bindings
+      - name: Pin proc-macro and quote to meet MSRV
+        run: |
+          cd c-bindings-gen
+          cargo update -p quote --precise "1.0.30" --verbose
+          cargo update -p proc-macro2 --precise "1.0.65" --verbose
       - name: Rebuild bindings without std, and check the sample app builds + links
         run: ./genbindings.sh ./rust-lightning false
       - name: Rebuild bindings, and check the sample app builds + links

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: std::io::Write>(w: &mut W, w_uses: &mut HashSet<String, NonRa
 									if let syn::Type::Reference(r) = &**rtype {
 										write!(w, "\n\t\t{}{}: ", $indent, $m.sig.ident).unwrap();
 										types.write_empty_rust_val(Some(&gen_types), w, &*r.elem);
-										writeln!(w, ",\n{}\t\tset_{}: Some({}_{}_set_{}),", $indent, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
+										writeln!(w, ".into(),\n{}\t\tset_{}: Some({}_{}_set_{}),", $indent, $m.sig.ident, ident, $trait.ident, $m.sig.ident).unwrap();
 										printed = true;
 									}
 								}
@@ -1198,9 +1198,9 @@ fn writeln_impl<W: std::io::Write>(w: &mut W, w_uses: &mut HashSet<String, NonRa
 										writeln!(w, "\t// This is a bit race-y in the general case, but for our specific use-cases today, we're safe").unwrap();
 										writeln!(w, "\t// Specifically, we must ensure that the first time we're called it can never be in parallel").unwrap();
 										write!(w, "\tif ").unwrap();
-										$types.write_empty_rust_val_check(Some(&meth_gen_types), w, &*r.elem, &format!("trait_self_arg.{}", $m.sig.ident));
+										$types.write_empty_rust_val_check(Some(&meth_gen_types), w, &*r.elem, &format!("unsafe {{ &*trait_self_arg.{}.get() }}", $m.sig.ident));
 										writeln!(w, " {{").unwrap();
-										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();
+										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();
 										writeln!(w, "\t}}").unwrap();
 										writeln!(w, "}}").unwrap();
 									}

genbindings.sh

@@ -231,13 +231,19 @@ cbindgen -v --config cbindgen.toml -o include/lightning.h >/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 <stdlib.h>/#include "ldk_rust_types.h"/g' include/lightning.h
 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 <stdlib.h>/#include "ldk_rust_types.h"/g' include/lightning.h

lightning-c-bindings/include/lightning.h

@@ -7884,7 +7884,7 @@ typedef struct LDKChannelSigner {
    /**
     * Returns the holder's channel public keys and basepoints.
     */
-   struct LDKChannelPublicKeys pubkeys;
+   LDKChannelPublicKeys pubkeys;
    /**
     * Fill in the pubkeys field as a reference to it will be given to Rust after this returns
     * Note that this takes a pointer to this object, not the this_ptr like other methods do

lightning-c-bindings/src/lightning/chain/keysinterface.rs

@@ -629,7 +629,7 @@ pub struct ChannelSigner {
 	#[must_use]
 	pub validate_holder_commitment: extern "C" fn (this_arg: *const c_void, holder_tx: &crate::lightning::ln::chan_utils::HolderCommitmentTransaction, preimages: crate::c_types::derived::CVec_PaymentPreimageZ) -> crate::c_types::derived::CResult_NoneNoneZ,
 	/// Returns the holder's channel public keys and basepoints.
-	pub pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys,
+	pub pubkeys: core::cell::UnsafeCell<crate::lightning::ln::chan_utils::ChannelPublicKeys>,
 	/// Fill in the pubkeys field as a reference to it will be given to Rust after this returns
 	/// Note that this takes a pointer to this object, not the this_ptr like other methods do
 	/// This function pointer may be NULL if pubkeys is filled in when this object is created and never needs updating.
@@ -662,7 +662,7 @@ pub(crate) extern "C" fn ChannelSigner_clone_fields(orig: &ChannelSigner) -> Cha
 		get_per_commitment_point: Clone::clone(&orig.get_per_commitment_point),
 		release_commitment_secret: Clone::clone(&orig.release_commitment_secret),
 		validate_holder_commitment: Clone::clone(&orig.validate_holder_commitment),
-		pubkeys: Clone::clone(&orig.pubkeys),
+		pubkeys: Clone::clone(unsafe { &*core::cell::UnsafeCell::get(&orig.pubkeys)}).into(),
 		set_pubkeys: Clone::clone(&orig.set_pubkeys),
 		channel_keys_id: Clone::clone(&orig.channel_keys_id),
 		provide_channel_parameters: Clone::clone(&orig.provide_channel_parameters),
@@ -690,7 +690,7 @@ impl rustChannelSigner for ChannelSigner {
 		if let Some(f) = self.set_pubkeys {
 			(f)(&self);
 		}
-		self.pubkeys.get_native_ref()
+		unsafe { &*self.pubkeys.get() }.get_native_ref()
 	}
 	fn channel_keys_id(&self) -> [u8; 32] {
 		let mut ret = (self.channel_keys_id)(self.this_arg);
@@ -889,7 +889,7 @@ impl lightning::chain::keysinterface::ChannelSigner for EcdsaChannelSigner {
 		if let Some(f) = self.ChannelSigner.set_pubkeys {
 			(f)(&self.ChannelSigner);
 		}
-		self.ChannelSigner.pubkeys.get_native_ref()
+		unsafe { &*self.ChannelSigner.pubkeys.get() }.get_native_ref()
 	}
 	fn channel_keys_id(&self) -> [u8; 32] {
 		let mut ret = (self.ChannelSigner.channel_keys_id)(self.ChannelSigner.this_arg);
@@ -1066,7 +1066,7 @@ impl lightning::chain::keysinterface::ChannelSigner for WriteableEcdsaChannelSig
 		if let Some(f) = self.EcdsaChannelSigner.ChannelSigner.set_pubkeys {
 			(f)(&self.EcdsaChannelSigner.ChannelSigner);
 		}
-		self.EcdsaChannelSigner.ChannelSigner.pubkeys.get_native_ref()
+		unsafe { &*self.EcdsaChannelSigner.ChannelSigner.pubkeys.get() }.get_native_ref()
 	}
 	fn channel_keys_id(&self) -> [u8; 32] {
 		let mut ret = (self.EcdsaChannelSigner.ChannelSigner.channel_keys_id)(self.EcdsaChannelSigner.ChannelSigner.this_arg);
@@ -1761,7 +1761,7 @@ pub extern "C" fn InMemorySigner_as_ChannelSigner(this_arg: &InMemorySigner) ->
 		release_commitment_secret: InMemorySigner_ChannelSigner_release_commitment_secret,
 		validate_holder_commitment: InMemorySigner_ChannelSigner_validate_holder_commitment,
 
-		pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true },
+		pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true }.into(),
 		set_pubkeys: Some(InMemorySigner_ChannelSigner_set_pubkeys),
 		channel_keys_id: InMemorySigner_ChannelSigner_channel_keys_id,
 		provide_channel_parameters: InMemorySigner_ChannelSigner_provide_channel_parameters,
@@ -1793,8 +1793,8 @@ extern "C" fn InMemorySigner_ChannelSigner_pubkeys(this_arg: *const c_void) -> c
 extern "C" fn InMemorySigner_ChannelSigner_set_pubkeys(trait_self_arg: &ChannelSigner) {
 	// This is a bit race-y in the general case, but for our specific use-cases today, we're safe
 	// Specifically, we must ensure that the first time we're called it can never be in parallel
-	if trait_self_arg.pubkeys.inner.is_null() {
-		unsafe { &mut *(trait_self_arg as *const ChannelSigner  as *mut ChannelSigner) }.pubkeys = InMemorySigner_ChannelSigner_pubkeys(trait_self_arg.this_arg);
+	if unsafe { &*trait_self_arg.pubkeys.get() }.inner.is_null() {
+		*unsafe { &mut *(&*(trait_self_arg as *const ChannelSigner)).pubkeys.get() } = InMemorySigner_ChannelSigner_pubkeys(trait_self_arg.this_arg).into();
 	}
 }
 #[must_use]
@@ -1839,7 +1839,7 @@ pub extern "C" fn InMemorySigner_as_EcdsaChannelSigner(this_arg: &InMemorySigner
 			release_commitment_secret: InMemorySigner_ChannelSigner_release_commitment_secret,
 			validate_holder_commitment: InMemorySigner_ChannelSigner_validate_holder_commitment,
 
-			pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true },
+			pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true }.into(),
 			set_pubkeys: Some(InMemorySigner_ChannelSigner_set_pubkeys),
 			channel_keys_id: InMemorySigner_ChannelSigner_channel_keys_id,
 			provide_channel_parameters: InMemorySigner_ChannelSigner_provide_channel_parameters,
@@ -1939,7 +1939,7 @@ pub extern "C" fn InMemorySigner_as_WriteableEcdsaChannelSigner(this_arg: &InMem
 				release_commitment_secret: InMemorySigner_ChannelSigner_release_commitment_secret,
 				validate_holder_commitment: InMemorySigner_ChannelSigner_validate_holder_commitment,
 
-				pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true },
+				pubkeys: crate::lightning::ln::chan_utils::ChannelPublicKeys { inner: core::ptr::null_mut(), is_owned: true }.into(),
 				set_pubkeys: Some(InMemorySigner_ChannelSigner_set_pubkeys),
 				channel_keys_id: InMemorySigner_ChannelSigner_channel_keys_id,
 				provide_channel_parameters: InMemorySigner_ChannelSigner_provide_channel_parameters,