From d9a38d1846ad64622706c3093c0bb3a405817899 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 24 Sep 2020 19:03:05 -0400 Subject: [PATCH] [bindings] Give Transaction objects a buffer-is-owned flag. A lot of our container mapping depends on the `is_owned` flag which we have for in-crate mapped objects to map references and non-references into the same container type. Transaction was mapped to two completely different types (a slice and a Vec type), which led to a number of edge cases in the bindings generation. Specifically, I spent a few days trying to map `[(A, &Transaction)]` properly and came up empty - we map slices into the same types as Vecs (and rely on the `is_owned` flag to avoid double-free) and the lack of one for `Transaction` would have required a special-case in numerous functions. Instead, we just add a flag in `Transaction` to mirror what we do for in-crate types and check it before free-ing any underlying memory. Note that, sadly, because the c_types objects aren't mapped as a part of our C++ bindings generation, you have to manually call `Transaction_free()` even in C++. --- c-bindings-gen/src/types.rs | 14 ++++------ lightning-c-bindings/demo.c | 1 + lightning-c-bindings/demo.cpp | 1 + lightning-c-bindings/src/c_types/mod.rs | 35 ++++++++++++++++++++----- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/c-bindings-gen/src/types.rs b/c-bindings-gen/src/types.rs index 30377c98a..1ddbe3cb9 100644 --- a/c-bindings-gen/src/types.rs +++ b/c-bindings-gen/src/types.rs @@ -340,8 +340,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> { "bitcoin::blockdata::script::Script" if is_ref => Some("crate::c_types::u8slice"), "bitcoin::blockdata::script::Script" if !is_ref => Some("crate::c_types::derived::CVec_u8Z"), "bitcoin::blockdata::transaction::OutPoint" if is_ref => Some("crate::chain::transaction::OutPoint"), - "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some("crate::c_types::Transaction"), - "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::derived::CVec_u8Z"), + "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::Transaction"), "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some("crate::c_types::TxOut"), "bitcoin::OutPoint" => Some("crate::chain::transaction::OutPoint"), "bitcoin::network::constants::Network" => Some("crate::bitcoin::network::Network"), @@ -413,7 +412,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> { "bitcoin::blockdata::script::Script" if is_ref => Some("&::bitcoin::blockdata::script::Script::from(Vec::from("), "bitcoin::blockdata::script::Script" if !is_ref => Some("::bitcoin::blockdata::script::Script::from("), "bitcoin::blockdata::transaction::Transaction" if is_ref => Some("&"), - "bitcoin::blockdata::transaction::Transaction" => Some("::bitcoin::consensus::encode::deserialize(&"), + "bitcoin::blockdata::transaction::Transaction" => Some(""), "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(""), "bitcoin::network::constants::Network" => Some(""), "bitcoin::blockdata::block::BlockHeader" => Some("&::bitcoin::consensus::encode::deserialize(unsafe { &*"), @@ -471,8 +470,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> { "bitcoin::secp256k1::key::SecretKey" if is_ref => Some("}[..]).unwrap()"), "bitcoin::blockdata::script::Script" if is_ref => Some(".to_slice()))"), "bitcoin::blockdata::script::Script" if !is_ref => Some(".into_rust())"), - "bitcoin::blockdata::transaction::Transaction" if is_ref => Some(".into_bitcoin()"), - "bitcoin::blockdata::transaction::Transaction" => Some(".into_rust()[..]).unwrap()"), + "bitcoin::blockdata::transaction::Transaction" => Some(".into_bitcoin()"), "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(".into_rust()"), "bitcoin::network::constants::Network" => Some(".into_bitcoin()"), "bitcoin::blockdata::block::BlockHeader" => Some(" }).unwrap()"), @@ -553,8 +551,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> { "bitcoin::secp256k1::Error" if !is_ref => Some("crate::c_types::Secp256k1Error::from_rust("), "bitcoin::blockdata::script::Script" if is_ref => Some("crate::c_types::u8slice::from_slice(&"), "bitcoin::blockdata::script::Script" if !is_ref => Some(""), - "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some("crate::c_types::Transaction::from_slice(&local_"), - "bitcoin::blockdata::transaction::Transaction" => Some("local_"), + "bitcoin::blockdata::transaction::Transaction" => Some("crate::c_types::Transaction::from_vec(local_"), "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some("crate::c_types::TxOut::from_rust("), "bitcoin::blockdata::block::BlockHeader" if is_ref => Some("&local_"), "bitcoin::blockdata::block::Block" if is_ref => Some("crate::c_types::u8slice::from_slice(&local_"), @@ -617,8 +614,7 @@ impl<'a, 'c: 'a> TypeResolver<'a, 'c> { "bitcoin::secp256k1::Error" if !is_ref => Some(")"), "bitcoin::blockdata::script::Script" if is_ref => Some("[..])"), "bitcoin::blockdata::script::Script" if !is_ref => Some(".into_bytes().into()"), - "bitcoin::blockdata::transaction::Transaction" if is_ref && !ptr_for_ref => Some(")"), - "bitcoin::blockdata::transaction::Transaction" => Some(".into()"), + "bitcoin::blockdata::transaction::Transaction" => Some(")"), "bitcoin::blockdata::transaction::TxOut" if !is_ref => Some(")"), "bitcoin::blockdata::block::BlockHeader" if is_ref => Some(""), "bitcoin::blockdata::block::Block" if is_ref => Some(")"), diff --git a/lightning-c-bindings/demo.c b/lightning-c-bindings/demo.c index 645c95bc4..44442ef3d 100644 --- a/lightning-c-bindings/demo.c +++ b/lightning-c-bindings/demo.c @@ -19,6 +19,7 @@ uint32_t get_fee(const void *this_arg, LDKConfirmationTarget target) { void broadcast_tx(const void *this_arg, LDKTransaction tx) { //TODO + Transaction_free(tx); } LDKCResult_NoneChannelMonitorUpdateErrZ add_channel_monitor(const void *this_arg, LDKOutPoint funding_txo, LDKChannelMonitor monitor) { diff --git a/lightning-c-bindings/demo.cpp b/lightning-c-bindings/demo.cpp index 7829f0265..8fa20209a 100644 --- a/lightning-c-bindings/demo.cpp +++ b/lightning-c-bindings/demo.cpp @@ -115,6 +115,7 @@ static int num_txs_broadcasted = 0; // Technically a race, but ints are atomic o void broadcast_tx(const void *this_arg, LDKTransaction tx) { num_txs_broadcasted += 1; //TODO + Transaction_free(tx); } struct NodeMonitors { diff --git a/lightning-c-bindings/src/c_types/mod.rs b/lightning-c-bindings/src/c_types/mod.rs index be697d496..4fc325f08 100644 --- a/lightning-c-bindings/src/c_types/mod.rs +++ b/lightning-c-bindings/src/c_types/mod.rs @@ -86,25 +86,48 @@ impl Secp256k1Error { } #[repr(C)] -/// A reference to a serialized transaction, in (pointer, length) form. -/// This type does *not* own its own memory, so access to it after, eg, the call in which it was -/// provided to you are invalid. +/// A serialized transaction, in (pointer, length) form. +/// +/// This type optionally owns its own memory, and thus the semantics around access change based on +/// the `data_is_owned` flag. If `data_is_owned` is set, you must call `Transaction_free` to free +/// the underlying buffer before the object goes out of scope. If `data_is_owned` is not set, any +/// access to the buffer after the scope in which the object was provided to you is invalid. eg, +/// access after you return from the call in which a `!data_is_owned` `Transaction` is provided to +/// you would be invalid. +/// +/// Note that, while it may change in the future, because transactions on the Rust side are stored +/// in a deserialized form, all `Transaction`s generated on the Rust side will have `data_is_owned` +/// set. Similarly, while it may change in the future, all `Transaction`s you pass to Rust may have +/// `data_is_owned` either set or unset at your discretion. pub struct Transaction { pub data: *const u8, pub datalen: usize, + pub data_is_owned: bool, } impl Transaction { pub(crate) fn into_bitcoin(&self) -> BitcoinTransaction { if self.datalen == 0 { panic!("0-length buffer can never represent a valid Transaction"); } ::bitcoin::consensus::encode::deserialize(unsafe { std::slice::from_raw_parts(self.data, self.datalen) }).unwrap() } - pub(crate) fn from_slice(s: &[u8]) -> Self { + pub(crate) fn from_vec(v: Vec) -> Self { + let datalen = v.len(); + let data = Box::into_raw(v.into_boxed_slice()); Self { - data: s.as_ptr(), - datalen: s.len(), + data: unsafe { (*data).as_mut_ptr() }, + datalen, + data_is_owned: true, } } } +impl Drop for Transaction { + fn drop(&mut self) { + if self.data_is_owned && self.datalen != 0 { + let _ = CVecTempl { data: self.data as *mut u8, datalen: self.datalen }; + } + } +} +#[no_mangle] +pub extern "C" fn Transaction_free(_res: Transaction) { } #[repr(C)] #[derive(Clone)] -- 2.39.5