xpay: ensure unique_id is always valid.
What the developer wrote
Authored by Rusty Russell
xpay: ensure unique_id is always valid.
In implementing shadow, I added a call to payment_log in xpay_core,
which caused a valgrind error because we set unique_id later. This
was a premature optimization, so we didn't assign unique_id for
payments which didn't even start, but it's a footgun, and there are
other (less common) calls to payment_log too early which make the same
mistake.
Simplify: unique_id is assigned at creation.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This is a code-quality fix inside Core Lightning's xpay plugin. It moves the assignment of a payment's unique ID earlier in the payment's lifetime so that logging functions can safely use it. The bug caused valgrind warnings (use of uninitialized data) and could have led to confusing log output, but it is not an exploitable security vulnerability.
No urgent action required. Treat as routine hardening. If running a build with xpay, include this commit to avoid uninitialized-memory reads and inconsistent log layer names.
Security signals we found
Use of uninitialized struct fields (unique_id, private_layer) before this patch
Valgrind error triggered by payment_log reading uninitialized unique_id
Fix is defensive hardening rather than a patch for an exploitable bug
Evidence from the diff
The commit removes the deferred payment_set_unique_id() helper and assigns payment->unique_id and payment->private_layer inside new_payment() at creation time. Previously these fields were set only after pre-approval succeeded, but payment_log() and other code paths could run before that point, reading uninitialized memory. The change also relocates a ‘No MPP support’ log message to xpay_core() where unique_id is now guaranteed to be valid. Test expectations are updated because the counter now advances for payments that never reach preapproval, changing observed layer names.
Changed components
plugins/xpay/xpay.ctests/test_xpay.pyInspect captured patch +8 / −21
diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c
index 0b4538ae..0f082e25 100644
--- a/plugins/xpay/xpay.c
+++ b/plugins/xpay/xpay.c
@@ -2060,16 +2060,6 @@ static struct command_result *param_string_array(struct command *cmd, const char
return NULL;
}
-/* Must be called after payment_new */
-static void payment_set_unique_id(struct payment *payment)
-{
- struct xpay *xpay = xpay_of(payment->plugin);
-
- payment->unique_id = xpay->counter++;
- payment->private_layer = tal_fmt(payment,
- "xpay-%"PRIu64, payment->unique_id);
-}
-
static struct command_result *
preapproveinvoice_succeed(struct command *cmd,
const char *method,
@@ -2082,12 +2072,6 @@ preapproveinvoice_succeed(struct command *cmd,
return command_check_done(cmd);
}
- payment_set_unique_id(payment);
-
- /* Now unique_id is set, we can log this message */
- if (payment->maxparts == 1)
- payment_log(payment, LOG_INFORM, "No MPP support: this is going to be hard to pay");
-
return populate_private_layer(cmd, payment);
}
@@ -2309,8 +2293,6 @@ static struct command_result *json_xpay_params(struct command *cmd,
}
/* Does NOT set:
- * ->unique_id
- * ->private_layer
* ->maxparts
* ->use_shadow
*
@@ -2401,6 +2383,9 @@ static struct payment *new_payment(const tal_t *ctx,
payment->requests = tal_arr(payment, struct out_req *, 0);
payment->start_time = clock_time();
payment->pay_compat = as_pay;
+ payment->unique_id = xpay->counter++;
+ payment->private_layer = tal_fmt(payment,
+ "xpay-%"PRIu64, payment->unique_id);
return payment;
}
@@ -2588,8 +2573,10 @@ static struct command_result *xpay_core(struct command *cmd,
else
payment->maxparts = 0;
- if (disable_mpp)
+ if (disable_mpp) {
payment->maxparts = 1;
+ payment_log(payment, LOG_INFORM, "No MPP support: this is going to be hard to pay");
+ }
/* Now preapprove, then start payment. */
if (command_check_only(cmd)) {
diff --git a/tests/test_xpay.py b/tests/test_xpay.py
index 33fef669..f708a54d 100644
--- a/tests/test_xpay.py
+++ b/tests/test_xpay.py
@@ -200,11 +200,11 @@ def test_xpay_simple(node_factory):
# Failure from l3 (with routehint)
l4.stop()
- with pytest.raises(RpcError, match=r"Failed after 1 attempts\. We got temporary_channel_failure for the invoice's route hint \([0-9x]*/[01]\), assuming it can't carry 10000msat\. Then routing failed: We could not find a usable set of paths\. The shortest path is [0-9x]*->[0-9x]*->[0-9x]*, but [0-9x]*/[01]\ layer xpay-7 says max is 9999msat"):
+ with pytest.raises(RpcError, match=r"Failed after 1 attempts\. We got temporary_channel_failure for the invoice's route hint \([0-9x]*/[01]\), assuming it can't carry 10000msat\. Then routing failed: We could not find a usable set of paths\. The shortest path is [0-9x]*->[0-9x]*->[0-9x]*, but [0-9x]*/[01]\ layer xpay-15 says max is 9999msat"):
l1.rpc.xpay(b11)
# Failure from l3 (with blinded path)
- with pytest.raises(RpcError, match=r"Failed after 1 attempts. We got an error from inside the blinded path 0x0x0/1: we assume it means insufficient capacity. Then routing failed: We could not find a usable set of paths. The shortest path is [0-9x]*->[0-9x]*->0x0x0, but 0x0x0/1 layer xpay-8 says max is 99999msat"):
+ with pytest.raises(RpcError, match=r"Failed after 1 attempts. We got an error from inside the blinded path 0x0x0/1: we assume it means insufficient capacity. Then routing failed: We could not find a usable set of paths. The shortest path is [0-9x]*->[0-9x]*->0x0x0, but 0x0x0/1 layer xpay-17 says max is 99999msat"):
l1.rpc.xpay(b12)
# Restart, try pay already paid one again.
Why this scored 22/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.