`as_ref()` before wrapping encoded types in writing `option` TLVs
What the developer wrote
Authored by Matt Corallo
`as_ref()` before wrapping encoded types in writing `option` TLVs
We almost certainly don't want to be moving `option` TLVs during
serialization, and while we had logic elsewhere to work around this
previously its nice not to have to in the future.
What changed, and why it matters
This commit changes how optional data fields are serialized in Lightning Dev Kit. Previously, the code could move (consume) values when wrapping them for encoding. Now it borrows them via `.as_ref()` first. The change is framed as a cleanup to avoid future bugs, but the prior workaround suggests there was a real risk of incorrect serialization or use-after-move behavior. There is no direct evidence of an exploitable vulnerability in the diff itself.
Review whether the prior workarounds fully mitigated any serialization-side effects, and confirm that the new borrowed implementations preserve byte-exact encoding. Consider adding regression tests for optional TLV serialization with non-Copy types. No immediate emergency action is indicated by the diff alone.
Security signals we found
Change prevents moving values during serialization of optional TLV fields
Adds Writeable impls for double-reference types to support borrowed encoding
Commit message references prior workarounds for the same issue
No explicit security claim or CVE in commit message
No vendor advisory or researcher attribution in supplied materials
Evidence from the diff
The patch modifies TLV serialization macros in ser_macros.rs so that optional fields use .as_ref().map(|f| $encoding(f)) instead of .map(|f| $encoding(f)). This prevents moving out of Option<T> values during length calculation and encoding. It also adds Writeable implementations for double-reference wrappers (&&String, &&Vec<T>, &&$features, HighZeroBytesDroppedBigSize<&$val_type>) and changes AccountableBool<bool> to AccountableBool<&bool>. These additions are needed because .as_ref() produces Option<&T>, and the existing encoders expected owned or single-reference types. The commit message says this avoids needing workarounds elsewhere, implying previous code had to compensate for the move behavior.
Changed components
lightning/src/util/ser_macros.rslightning/src/util/ser.rslightning/src/ln/msgs.rslightning/src/ln/features.rsInspect captured patch +33 / −5
diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index b568d55..a4e7fc1 100644
--- a/lightning/src/ln/features.rs
+++ b/lightning/src/ln/features.rs
@@ -81,6 +81,12 @@ macro_rules! impl_feature_write_without_length {
}
}
+ impl Writeable for WithoutLength<&&$features> {
+ fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+ write_be(w, self.0.le_flags())
+ }
+ }
+
impl Readable for WithoutLength<$features> {
fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
let v = io_extras::read_to_end(r)?;
diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs
index 6210d26..5643bfd 100644
--- a/lightning/src/ln/msgs.rs
+++ b/lightning/src/ln/msgs.rs
@@ -763,10 +763,10 @@ pub struct UpdateAddHTLC {
struct AccountableBool<T>(T);
-impl Writeable for AccountableBool<bool> {
+impl Writeable for AccountableBool<&bool> {
#[inline]
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
- let wire_value = if self.0 { 7u8 } else { 0u8 };
+ let wire_value = if *self.0 { 7u8 } else { 0u8 };
writer.write_all(&[wire_value])
}
}
diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index bd2488b..4c40382 100644
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -610,6 +610,13 @@ macro_rules! impl_writeable_primitive {
writer.write_all(&self.0.to_be_bytes()[(self.0.leading_zeros() / 8) as usize..$len])
}
}
+ impl Writeable for HighZeroBytesDroppedBigSize<&$val_type> {
+ #[inline]
+ fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+ // Skip any full leading 0 bytes when writing (in BE):
+ writer.write_all(&self.0.to_be_bytes()[(self.0.leading_zeros() / 8) as usize..$len])
+ }
+ }
impl Readable for $val_type {
#[inline]
fn read<R: Read>(reader: &mut R) -> Result<$val_type, DecodeError> {
@@ -751,12 +758,20 @@ impl_array!(HMAC_LEN * HMAC_COUNT, u8);
/// This is not exported to bindings users as manual TLV building is not currently supported in bindings
pub struct WithoutLength<T>(pub T);
+impl Writeable for WithoutLength<&&String> {
+ #[inline]
+ fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+ w.write_all(self.0.as_bytes())
+ }
+}
+
impl Writeable for WithoutLength<&String> {
#[inline]
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
w.write_all(self.0.as_bytes())
}
}
+
impl LengthReadable for WithoutLength<String> {
#[inline]
fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
@@ -808,6 +823,14 @@ impl<T: Writeable> AsWriteableSlice for &Vec<T> {
&self
}
}
+
+impl<T: Writeable> AsWriteableSlice for &&Vec<T> {
+ type Inner = T;
+ fn as_slice(&self) -> &[T] {
+ &self
+ }
+}
+
impl<T: Writeable> AsWriteableSlice for &[T] {
type Inner = T;
fn as_slice(&self) -> &[T] {
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 946be54..c023ab4 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -81,7 +81,7 @@ macro_rules! _encode_tlv {
$crate::_encode_tlv!($stream, $type, $field, option);
};
($stream: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident) $(, $self: ident)?)) => {
- $crate::_encode_tlv!($stream, $type, $field.map(|f| $encoding(f)), option);
+ $crate::_encode_tlv!($stream, $type, $field.as_ref().map(|f| $encoding(f)), option);
};
($stream: expr, $type: expr, $field: expr, (option, encoding: $fieldty: ty) $(, $self: ident)?) => {
$crate::_encode_tlv!($stream, $type, $field, option);
@@ -253,8 +253,7 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, option);
};
($len: expr, $type: expr, $field: expr, (option, encoding: ($fieldty: ty, $encoding: ident)) $(, $self: ident)?) => {
- let field = $field.map(|f| $encoding(f));
- $crate::_get_varint_length_prefixed_tlv_length!($len, $type, field, option);
+ $crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field.as_ref().map(|f| $encoding(f)), option);
};
($len: expr, $type: expr, $field: expr, upgradable_required $(, $self: ident)?) => {
$crate::_get_varint_length_prefixed_tlv_length!($len, $type, $field, required);
Why this scored 35/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.