askrene: fix crash loading node bias with description - #9434
askrene: fix crash loading node bias with description#9434vincenzopalazzo wants to merge 2 commits into
Conversation
test_node_bias_persistence() restarted l2, but the layer and its node bias records live in l1's datastore. The assert compared l1's in-memory layer against itself, so load_node_bias() was never actually exercised and this test could not have caught the startup crash in ElementsProject#9433. Restarting l1 instead makes the test reload the layer from the datastore at startup. This is the reproducer for ElementsProject#9433; it fails until the next commit. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
load_node_bias() passed take(description) to two consecutive set_node_bias() calls. The first call's tal_strdup() consumes the take (tal_resize_ + tal_steal), so the second take() was on freed memory and we aborted in to_tal_hdr() with "Not a valid header" while loading the layer at startup. Since askrene is an important plugin, lightningd shuts down and the node cannot restart at all. The description is already a copy off tmpctx, so simply don't take() it: set_node_bias() strdups it into the bias anyway. With this, the test from the previous commit passes. Fixes: ElementsProject#9433 Reported-by: endothermicdev Changelog-Fixed: askrene: node failed to start (`exited before replying to init`) when a persistent layer contains a node bias with a description Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
011b487 to
5fc57d9
Compare
Andezion
left a comment
There was a problem hiding this comment.
set_node_bias() keys node_bias by node ID only (node_bias_hash_get(layer->node_biases, node)), so both calls in load_node_bias() (in-bias and out-bias) resolve to the same struct. That means the second call still does tal_free(bias->description) and re tal_strdups the identical string that the first call just set. Is that worth folding into a single "set both biases" helper at some point? What do you think?
|
Good point: both calls hit the same node_bias, so the second one frees and re-strdups the identical description. But that's once per record at startup, and the one-direction API is what the RPC actually wants: askrene-bias-node sets a single side, and the both-zero-removes-the-entry rule lives inside set_node_bias(). A "set both" helper would end up duplicating that, which seems worse than one wasted strdup on load. If we do care, the cheap fix is to pass NULL for the description on the in call and let the out call carry it: exactly one strdup, no new API. Happy to fold that in here. |
Fixes #9433.
Two commits, red then green: the first makes the existing
test_node_bias_persistenceactually exercise layer persistence (it failson master with the exact crash below), the second fixes the crash.
Commit 1: tests only — the reproducer
test_node_bias_persistencerestartedl2, but the layer and its nodebias records live in
l1's datastore: the assert was comparing l1'sin-memory layer against itself, so
load_node_bias()never ran. Withl1.restart()the layer is reloaded from the datastore at startup, whichtriggers the crash on master:
Commit 2: the fix
load_node_bias()passedtake(description)to two consecutiveset_node_bias()calls. The first call'stal_strdup()consumesthe take (
tal_resize_+tal_stealinsidetal_dup_), so thesecond
take()was on freed memory and we aborted into_tal_hdr()with "Not a valid header" while loading the layer atstartup:
Since lightningd treats askrene as an important plugin, any persistent
layer containing a node bias with a description makes the node unable to
restart.
The description is already a copy off tmpctx, so the simplest thing:
don't take() it,
set_node_bias()strdups it into the bias anyway(as the issue notes, it only needs loading once).
Verified locally
pytest tests/test_askrene.py::test_node_bias_persistencefails — node crashes at restart (
FATAL SIGNAL 6,exited before replying to init).(description, in_bias, out_bias, timestamp).