← Watch feed
lightningd: fix scb remote_to_self_delay information.
Public commit record
81/100 · Strong
What the developer wrote
Authored by Rusty Russell
lightningd: fix scb remote_to_self_delay information.
This was changing all the time when I tried to make
autogenerate-rpc-examples.py reproducible. Turns out it was being
corrupted (it does suspicious things with pointers); rather than try
to diagnose it, I simply rewrote the code to create it only when we
need it.
```
Valgrind error file: valgrind-errors.34506
==34506== Uninitialised byte(s) found during client check request
==34506== at 0x241732: memcheck_ (mem.h:247)
==34506== by 0x2417BC: towire (towire.c:17)
==34506== by 0x24185C: towire_u16 (towire.c:28)
==34506== by 0x20C8E4: towire_tlv_scb_tlvs_remote_to_self_delay (scb_wiregen.c:213)
==34506== by 0x240E78: towire_tlv (tlvstream.c:342)
==34506== by 0x20C99F: towire_tlv_scb_tlvs (scb_wiregen.c:234)
==34506== by 0x20C298: towire_modern_scb_chan (scb_wiregen.c:89)
==34506== by 0x1A6CF3: json_add_scb (peer_control.c:2488)
==34506== by 0x1A6E0C: json_staticbackup (peer_control.c:2519)
==34506== by 0x177E3F: command_exec (jsonrpc.c:799)
==34506== by 0x1785AE: rpc_command_hook_final (jsonrpc.c:945)
==34506== by 0x1BEC2D: plugin_hook_call_next (plugin_hook.c:199)
==34506== Address 0x1ffeffe736 is on thread 1's stack
==34506== in frame #2, created by towire_u16 (towire.c:26)
==34506==
{
<insert_a_suppression_name_here>
Memcheck:User
fun:memcheck_
fun:towire
fun:towire_u16
fun:towire_tlv_scb_tlvs_remote_to_self_delay
fun:towire_tlv
fun:towire_tlv_scb_tlvs
fun:towire_modern_scb_chan
fun:json_add_scb
fun:json_staticbackup
fun:command_exec
fun:rpc_command_hook_final
fun:plugin_hook_call_next
}
==34506== Uninitialised byte(s) found during client check request
==34506== at 0x241732: memcheck_ (mem.h:247)
==34506== by 0x2417BC: towire (towire.c:17)
==34506== by 0x240EF0: towire_tlv (tlvstream.c:354)
==34506== by 0x20C99F: towire_tlv_scb_tlvs (scb_wiregen.c:234)
==34506== by 0x20C298: towire_modern_scb_chan (scb_wiregen.c:89)
==34506== by 0x1A6CF3: json_add_scb (peer_control.c:2488)
==34506== by 0x1A6E0C: json_staticbackup (peer_control.c:2519)
==34506== by 0x177E3F: command_exec (jsonrpc.c:799)
==34506== by 0x1785AE: rpc_command_hook_final (jsonrpc.c:945)
==34506== by 0x1BEC2D: plugin_hook_call_next (plugin_hook.c:199)
==34506== by 0x1BEBA8: plugin_hook_callback (plugin_hook.c:186)
==34506== by 0x1B771E: plugin_response_handle (plugin.c:705)
==34506== Address 0x7bd1d08 is 40 bytes inside a block of size 42 alloc'd
==34506== at 0x484DCD3: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==34506== by 0x3C8614: tal_resize_ (tal.c:755)
==34506== by 0x2417A2: towire (towire.c:14)
==34506== by 0x24185C: towire_u16 (towire.c:28)
==34506== by 0x20C8E4: towire_tlv_scb_tlvs_remote_to_self_delay (scb_wiregen.c:213)
==34506== by 0x240E78: towire_tlv (tlvstream.c:342)
==34506== by 0x20C99F: towire_tlv_scb_tlvs (scb_wiregen.c:234)
==34506== by 0x20C298: towire_modern_scb_chan (scb_wiregen.c:89)
==34506== by 0x1A6CF3: json_add_scb (peer_control.c:2488)
==34506== by 0x1A6E0C: json_staticbackup (peer_control.c:2519)
==34506== by 0x177E3F: command_exec (jsonrpc.c:799)
==34506== by 0x1785AE: rpc_command_hook_final (jsonrpc.c:945)
==34506==
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Links an issue, advisory, or supporting reference
What changed, and why it matters
This commit fixes a bug in Core Lightning's static channel backup (SCB) feature. Previously, the backup data structure was stored inside the channel object and contained pointers to other channel fields. Those pointers could become stale or point to uninitialized memory, causing the backup to include corrupt or changing data. The fix rebuilds the backup data fresh each time it is requested, using current channel values, which removes the stale-pointer problem. The commit message and Valgrind output show uninitialized bytes being serialized into the backup, but the bug is described as corruption rather than a deliberate security vulnerability.
Recommended action
Apply the patch. After applying, run the staticbackup RPC under Valgrind or AddressSanitizer with channels in various states to confirm no uninitialized bytes are serialized. Review other long-lived objects that hold pointers to channel sub-fields for similar lifetime issues. Consider adding a regression test that compares two consecutive staticbackup outputs for the same channel and asserts they are identical.
Security signals we found
01
Use of stale pointers to channel fields in serialized backup data
02
Valgrind-reported uninitialized bytes in towire path for remote_to_self_delay
03
Backup payload contents could vary nondeterministically (observed during autogenerate-rpc-examples.py)
04
Memory safety issue in TLV serialization of channel backup
05
No explicit bounds or lifetime issue, but pointer lifetime mismatch between channel and embedded SCB
Evidence from the diff
The patch removes the channel->scb field and all code that eagerly constructs a modern_scb_chan object when a channel is created. Instead, json_add_scb() now allocates a fresh modern_scb_chan on tmpctx, populates it from the live struct channel, serializes it, and then lets it be freed. This eliminates the earlier pattern where scb_tlvs members such as remote_to_self_delay, shachain, basepoints, and opener were pointers to channel fields. The Valgrind trace shows uninitialized bytes being written while serializing remote_to_self_delay, which the author attributes to pointer corruption rather than a root cause. The change also switches the staticbackup RPC loop to skip uncommitted channels instead of relying on a non-null scb.
Changed components
lightningd/channel.clightningd/channel.hlightningd/dual_open_control.clightningd/peer_control.clightningd/test/run-invoice-select-inchan.cstaticbackup RPCmodern_scb_chan TLV serializationInspect captured patch +35 / −57
diff --git a/lightningd/channel.c b/lightningd/channel.c
index 354249a4..1390a486 100644
--- a/lightningd/channel.c
+++ b/lightningd/channel.c
@@ -343,7 +343,6 @@ struct channel *new_unsaved_channel(struct peer *peer,
channel->openchannel_signed_cmd = NULL;
channel->state = DUALOPEND_OPEN_INIT;
channel->owner = NULL;
- channel->scb = NULL;
channel->reestablished = false;
memset(&channel->billboard, 0, sizeof(channel->billboard));
channel->billboard.transient = tal_fmt(channel, "%s",
@@ -578,30 +577,6 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
channel->billboard.transient = tal_strdup(channel, transient_billboard);
channel->channel_info = *channel_info;
- /* If it's a unix domain socket connection, we don't save it */
- if (peer->addr.itype == ADDR_INTERNAL_WIREADDR) {
- channel->scb = tal(channel, struct modern_scb_chan);
- channel->scb->id = dbid;
- /* More useful to have last_known_addr, if avail */
- if (peer->last_known_addr)
- channel->scb->addr = *peer->last_known_addr;
- channel->scb->addr = peer->addr.u.wireaddr.wireaddr;
- channel->scb->node_id = peer->id;
- channel->scb->funding = *funding;
- channel->scb->cid = *cid;
- channel->scb->funding_sats = funding_sats;
- channel->scb->type = channel_type_dup(channel->scb, type);
-
- struct tlv_scb_tlvs *scb_tlvs = tlv_scb_tlvs_new(channel);
- scb_tlvs->shachain = &channel->their_shachain.chain;
- scb_tlvs->basepoints = &channel->channel_info.theirbase;
- scb_tlvs->opener = &channel->opener;
- scb_tlvs->remote_to_self_delay = &channel->channel_info.their_config.to_self_delay;
-
- channel->scb->tlvs = scb_tlvs;
- } else
- channel->scb = NULL;
-
if (!log) {
channel->log = new_logger(channel,
peer->ld->log_book,
diff --git a/lightningd/channel.h b/lightningd/channel.h
index af2f18a5..fd8e031b 100644
--- a/lightningd/channel.h
+++ b/lightningd/channel.h
@@ -343,10 +343,6 @@ struct channel {
/* Lease commited max part per thousandth channel fee (ppm * 1000) */
u16 lease_chan_max_ppt;
- /* `Channel-shell` of this channel
- * (Minimum information required to backup this channel). */
- struct modern_scb_chan *scb;
-
/* Do we allow the peer to set any fee it wants? */
bool ignore_fee_limits;
diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c
index 1e46c06a..ab84e3f4 100644
--- a/lightningd/dual_open_control.c
+++ b/lightningd/dual_open_control.c
@@ -1467,28 +1467,9 @@ wallet_commit_channel(struct lightningd *ld,
&commitment_feerate);
channel->min_possible_feerate = commitment_feerate;
channel->max_possible_feerate = commitment_feerate;
- if (channel->peer->addr.itype == ADDR_INTERNAL_WIREADDR) {
- channel->scb = tal(channel, struct modern_scb_chan);
- channel->scb->id = channel->dbid;
- channel->scb->addr = channel->peer->addr.u.wireaddr.wireaddr;
- channel->scb->node_id = channel->peer->id;
- channel->scb->funding = *funding;
- channel->scb->cid = channel->cid;
- channel->scb->funding_sats = total_funding;
-
- struct tlv_scb_tlvs *scb_tlvs = tlv_scb_tlvs_new(channel);
- scb_tlvs->shachain = &channel->their_shachain.chain;
- scb_tlvs->basepoints = &channel_info->theirbase;
- scb_tlvs->opener = &channel->opener;
- scb_tlvs->remote_to_self_delay = &channel_info->their_config.to_self_delay;
-
- channel->scb->tlvs = scb_tlvs;
- } else
- channel->scb = NULL;
tal_free(channel->type);
channel->type = channel_type_dup(channel, type);
- channel->scb->type = channel_type_dup(channel->scb, type);
if (our_upfront_shutdown_script)
channel->shutdown_scriptpubkey[LOCAL]
diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c
index 19780fae..fa458cb2 100644
--- a/lightningd/peer_control.c
+++ b/lightningd/peer_control.c
@@ -2480,15 +2480,35 @@ static void json_add_scb(struct command *cmd,
struct json_stream *response,
struct channel *c)
{
- u8 *scb = tal_arr(cmd, u8, 0);
+ u8 *scb_wire = tal_arr(cmd, u8, 0);
+ struct modern_scb_chan *scb;
- /* Update shachain & basepoints in SCB. */
- c->scb->tlvs->shachain = &c->their_shachain.chain;
- c->scb->tlvs->basepoints = &c->channel_info.theirbase;
- towire_modern_scb_chan(&scb, c->scb);
+ /* Don't do scb for unix domain sockets. */
+ if (c->peer->addr.itype != ADDR_INTERNAL_WIREADDR)
+ return;
+
+ scb = tal(tmpctx, struct modern_scb_chan);
+ scb->id = c->dbid;
+ /* More useful to have last_known_addr, if avail */
+ if (c->peer->last_known_addr)
+ scb->addr = *c->peer->last_known_addr;
+ else
+ scb->addr = c->peer->addr.u.wireaddr.wireaddr;
+ scb->node_id = c->peer->id;
+ scb->funding = c->funding;
+ scb->cid = c->cid;
+ scb->funding_sats = c->funding_sats;
+ scb->type = channel_type_dup(scb, c->type);
+
+ scb->tlvs = tlv_scb_tlvs_new(scb);
+ scb->tlvs->shachain = &c->their_shachain.chain;
+ scb->tlvs->basepoints = &c->channel_info.theirbase;
+ scb->tlvs->opener = &c->opener;
+ scb->tlvs->remote_to_self_delay = &c->channel_info.their_config.to_self_delay;
+
+ towire_modern_scb_chan(&scb_wire, scb);
- json_add_hex_talarr(response, fieldname,
- scb);
+ json_add_hex_talarr(response, fieldname, scb_wire);
}
/* This will return a SCB for all the channels currently loaded
@@ -2513,8 +2533,7 @@ static struct command_result *json_staticbackup(struct command *cmd,
peer = peer_node_id_map_next(cmd->ld->peers, &it)) {
struct channel *channel;
list_for_each(&peer->channels, channel, list){
- /* cppcheck-suppress uninitvar - false positive on channel */
- if (!channel->scb)
+ if (channel_state_uncommitted(channel->state))
continue;
json_add_scb(cmd, NULL, response, channel);
}
diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c
index 00eef5e4..44e8c5d7 100644
--- a/lightningd/test/run-invoice-select-inchan.c
+++ b/lightningd/test/run-invoice-select-inchan.c
@@ -144,6 +144,10 @@ const char *channel_state_name(const struct channel *channel UNNEEDED)
/* Generated stub for channel_state_str */
const char *channel_state_str(enum channel_state state UNNEEDED)
{ fprintf(stderr, "channel_state_str called!\n"); abort(); }
+/* Generated stub for channel_type_dup */
+struct channel_type *channel_type_dup(const tal_t *ctx UNNEEDED,
+ const struct channel_type *t UNNEEDED)
+{ fprintf(stderr, "channel_type_dup called!\n"); abort(); }
/* Generated stub for channel_type_has */
bool channel_type_has(const struct channel_type *type UNNEEDED, int feature UNNEEDED)
{ fprintf(stderr, "channel_type_has called!\n"); abort(); }
@@ -969,6 +973,9 @@ void subd_send_fd(struct subd *sd UNNEEDED, int fd UNNEEDED)
/* Generated stub for subd_send_msg */
void subd_send_msg(struct subd *sd UNNEEDED, const u8 *msg_out UNNEEDED)
{ fprintf(stderr, "subd_send_msg called!\n"); abort(); }
+/* Generated stub for tlv_scb_tlvs_new */
+struct tlv_scb_tlvs *tlv_scb_tlvs_new(const tal_t *ctx UNNEEDED)
+{ fprintf(stderr, "tlv_scb_tlvs_new called!\n"); abort(); }
/* Generated stub for towire_bigsize */
void towire_bigsize(u8 **pptr UNNEEDED, const bigsize_t val UNNEEDED)
{ fprintf(stderr, "towire_bigsize called!\n"); abort(); }
Risk score
Our methodology →Why this scored 48/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.