lightningd: use htable lock/unlock to ensure we don't add to htables while iterating.
What the developer wrote
Authored by Rusty Russell
lightningd: use htable lock/unlock to ensure we don't add to htables while iterating.
Not all of them, but I've done the ones which weren't obvious.
Also, there's never a problem deleting during iteration, so remove places which
tried to "handle" that problem.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This change adds safety locks around hash table loops in Core Lightning so the code cannot accidentally insert new entries while it is still walking through the same table. It also removes some old 'rerun' work-arounds that were meant to cope with entries being skipped during deletion. The patch is a hardening fix: it makes the daemon more robust against internal corruption or crashes caused by modifying a hash table while iterating over it, but it does not by itself add or remove any user-facing feature.
Treat as a stability and defensive-security improvement. Review remaining htable iteration sites that the author describes as not yet covered. Verify that the lock/unlock primitives are present in the build and that assertions are enabled in release builds, otherwise the protection may be a no-op.
Security signals we found
Hash-table iterator invalidation hardening
Addition-during-iteration now asserted against via lock/unlock
Removal of defensive rerun loops based on assumption that deletion-during-iteration is safe
Memory-safety / crash-avoidance class fix
Evidence from the diff
The commit introduces htable lock/unlock calls around several iteration sites over peer_node_id_map, replay_tx_hash, and txwatch_hash. The lock primitives assert if code tries to add to the htable during iteration, preventing iterator invalidation. The patch also updates comments and removes the ‘goto again’ loop in free_all_channels and the needs_rerun loop in watch_topology_changed, because deletion during iteration is now considered safe and consistent. The change is defensive and partial: the commit message says ‘Not all of them, but I’ve done the ones which weren’t obvious.’
Changed components
lightningd/channel_gossip.clightningd/lightningd.clightningd/onchain_control.clightningd/watch.cInspect captured patch +28 / −21
diff --git a/lightningd/channel_gossip.c b/lightningd/channel_gossip.c
index d8dab441..cb5a42d8 100644
--- a/lightningd/channel_gossip.c
+++ b/lightningd/channel_gossip.c
@@ -1064,6 +1064,8 @@ void channel_gossip_notify_new_block(struct lightningd *ld)
struct channel *channel;
struct peer_node_id_map_iter it;
+ /* No addition during iteration! */
+ peer_node_id_map_lock(ld->peers);
for (peer = peer_node_id_map_first(ld->peers, &it);
peer;
peer = peer_node_id_map_next(ld->peers, &it)) {
@@ -1075,6 +1077,7 @@ void channel_gossip_notify_new_block(struct lightningd *ld)
new_blockheight(ld, channel);
}
}
+ peer_node_id_map_unlock(ld->peers);
}
/* Gossipd told us about a channel update on one of our channels (on loading) */
diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
index f23fb7df..97c5461a 100644
--- a/lightningd/lightningd.c
+++ b/lightningd/lightningd.c
@@ -616,10 +616,11 @@ static void free_all_channels(struct lightningd *ld)
* given a destructor (`destroy_peer`) which removes itself from the
* hashtable.
*
- * Deletion from a hashtable is allowed, but it does mean we could
- * skip entries in iteration. Hence we repeat until empty!
+ * Deletion from a hashtable during iteration is safe and consistent.
+ * Adding is forbidden, hence the lock() function which causes that to
+ * assert.
*/
-again:
+ peer_node_id_map_lock(ld->peers);
for (p = peer_node_id_map_first(ld->peers, &it);
p;
p = peer_node_id_map_next(ld->peers, &it)) {
@@ -644,8 +645,7 @@ again:
/* Removes itself from htable as we free it */
tal_free(p);
}
- if (peer_node_id_map_first(ld->peers, &it))
- goto again;
+ peer_node_id_map_unlock(ld->peers);
/*~ Commit the transaction. Note that the db is actually
* single-threaded, so commits never fail and we don't need
diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c
index 8d7f99f4..2a0fa583 100644
--- a/lightningd/onchain_control.c
+++ b/lightningd/onchain_control.c
@@ -386,11 +386,13 @@ static void convert_replay_txs(struct channel *channel)
/* Set to NULL so these are queued as real watches */
watches = tal_steal(tmpctx, channel->onchaind_replay_watches);
channel->onchaind_replay_watches = NULL;
+ replay_tx_hash_lock(watches);
for (rtx = replay_tx_hash_first(watches, &rit);
rtx;
rtx = replay_tx_hash_next(watches, &rit)) {
watch_tx_and_outputs(channel, rtx->tx);
}
+ replay_tx_hash_unlock(watches);
}
static void replay_block(struct bitcoind *bitcoind,
@@ -407,12 +409,14 @@ static void replay_block(struct bitcoind *bitcoind,
return;
/* Tell onchaind that all existing txs have reached a new depth */
+ replay_tx_hash_lock(channel->onchaind_replay_watches);
for (rtx = replay_tx_hash_first(channel->onchaind_replay_watches, &rit);
rtx;
rtx = replay_tx_hash_next(channel->onchaind_replay_watches, &rit)) {
/* Note: if you're in this block, that's depth 1! */
onchain_tx_depth(channel, &rtx->txid, height - rtx->blockheight + 1);
}
+ replay_tx_hash_unlock(channel->onchaind_replay_watches);
/* See if we add any new txs which spend a watched one */
for (size_t i = 0; i < tal_count(blk->tx); i++) {
diff --git a/lightningd/watch.c b/lightningd/watch.c
index 67b2bb08..0731f711 100644
--- a/lightningd/watch.c
+++ b/lightningd/watch.c
@@ -275,24 +275,24 @@ void watch_topology_changed(struct chain_topology *topo)
{
struct txwatch_hash_iter i;
struct txwatch *w;
- bool needs_rerun;
- do {
- /* Iterating a htable during deletes is safe, but might skip entries. */
- needs_rerun = false;
- for (w = txwatch_hash_first(topo->txwatches, &i);
- w;
- w = txwatch_hash_next(topo->txwatches, &i)) {
- u32 depth;
-
- depth = get_tx_depth(topo, &w->txid);
- if (depth) {
- if (!w->tx)
- w->tx = wallet_transaction_get(w, topo->ld->wallet,
+
+ /* Iterating a htable during deletes is safe and consistent.
+ * Adding is forbidden. */
+ txwatch_hash_lock(topo->txwatches);
+ for (w = txwatch_hash_first(topo->txwatches, &i);
+ w;
+ w = txwatch_hash_next(topo->txwatches, &i)) {
+ u32 depth;
+
+ depth = get_tx_depth(topo, &w->txid);
+ if (depth) {
+ if (!w->tx)
+ w->tx = wallet_transaction_get(w, topo->ld->wallet,
&w->txid);
- needs_rerun |= txw_fire(w, &w->txid, depth);
- }
+ txw_fire(w, &w->txid, depth);
}
- } while (needs_rerun);
+ }
+ txwatch_hash_unlock(topo->txwatches);
}
void txwatch_inform(const struct chain_topology *topo,
Why this scored 44/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.