← Watch feed
plugins/bkpr/test/run-recorder: don't hand NULL cmd.
Public commit record
75/100 · Adequate
What the developer wrote
Authored by Rusty Russell
plugins/bkpr/test/run-recorder: don't hand NULL cmd.
ubsan complains that we declared a function not to take NULL.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides an explanatory body✓ Mentions testing or verification
What changed, and why it matters
This is a fix inside a test program for the bookkeeping plugin. The test was passing NULL (an empty pointer) to functions that are declared to never accept NULL, which caused the Undefined Behavior Sanitizer (UBSan) to complain during testing. The change creates a dummy command object and passes it instead. It does not affect the actual Core Lightning node software that users run.
Recommended action
No production action required. Treat as a normal test-quality/CI hygiene patch. If reviewing, confirm that the dummy cmd object is properly allocated and freed or accounted for in the test's tal context to avoid a minor memory leak in the test runner.
Security signals we found
01
Undefined Behavior Sanitizer (UBSan) non-null attribute violation in test code
02
NULL pointer replaced with allocated dummy object in test harness only
03
No changes to production plugin or daemon code
Evidence from the diff
The commit modifies plugins/bkpr/test/run-recorder.c, a unit-test harness for the bookkeeper plugin. It adds a static struct command *cmd and initializes it with tal(tmpctx, struct command) before running tests, then replaces all NULL command arguments in test calls with this non-NULL cmd. The functions involved (account_datastore_set, maybe_update_onchain_fees, maybe_update_account, account_onchain_closeheight, account_update_closeheight, update_channel_onchain_fees, account_get_channel_events, maybe_record_rebalance) are annotated so UBSan treats NULL as a contract violation. The change is purely test-hardening; no production code paths are altered.
Changed components
plugins/bkpr/test/run-recorder.cInspect captured patch +54 / −51
diff --git a/plugins/bkpr/test/run-recorder.c b/plugins/bkpr/test/run-recorder.c
index a8b131e1..af8802e6 100644
--- a/plugins/bkpr/test/run-recorder.c
+++ b/plugins/bkpr/test/run-recorder.c
@@ -90,6 +90,7 @@ void plugin_log(struct plugin *p UNNEEDED, enum log_level l UNNEEDED, const char
/* AUTOGENERATED MOCKS END */
static sqlite3 *bkpr_db;
+static struct command *cmd;
/* Stolen from old plugins/bkpr/db.c */
struct migration {
@@ -734,8 +735,8 @@ static bool test_onchain_fee_wallet_spend(const tal_t *ctx)
memset(&txid, '1', sizeof(struct bitcoin_txid));
db_begin_transaction(db);
- account_datastore_set(NULL, wal_acct, "must-create");
- account_datastore_set(NULL, ext_acct, "must-create");
+ account_datastore_set(cmd, wal_acct, "must-create");
+ account_datastore_set(cmd, ext_acct, "must-create");
db_commit_transaction(db);
@@ -753,7 +754,7 @@ static bool test_onchain_fee_wallet_spend(const tal_t *ctx)
AMOUNT_MSAT(1000),
blockheight,
'X', 0, '1'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, wal_acct,
make_chain_event(ctx, "deposit",
@@ -762,7 +763,7 @@ static bool test_onchain_fee_wallet_spend(const tal_t *ctx)
AMOUNT_MSAT(200),
blockheight,
'1', 1, '*'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, ext_acct,
make_chain_event(ctx, "deposit",
@@ -771,7 +772,7 @@ static bool test_onchain_fee_wallet_spend(const tal_t *ctx)
AMOUNT_MSAT(700),
blockheight,
'1', 0, '*'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
db_commit_transaction(db);
ofs = list_chain_fees(ctx, bkpr);
@@ -815,9 +816,9 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
acct->peer_id = &peer_id;
db_begin_transaction(db);
- account_datastore_set(NULL, wal_acct, "must-create");
- account_datastore_set(NULL, ext_acct, "must-create");
- account_datastore_set(NULL, acct, "must-create");
+ account_datastore_set(cmd, wal_acct, "must-create");
+ account_datastore_set(cmd, ext_acct, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
db_commit_transaction(db);
/* Close a channel */
@@ -845,7 +846,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
'X', 0, '*');
log_chain_event(bkpr, acct, ev);
tags[0] = MVT_CHANNEL_OPEN;
- maybe_update_account(NULL, acct, ev, tags, 0, NULL);
+ maybe_update_account(cmd, acct, ev, tags, 0, NULL);
ev = make_chain_event(ctx, "channel_close",
AMOUNT_MSAT(0),
@@ -857,7 +858,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
/* Update the account to have the right info! */
tags[0] = MVT_CHANNEL_CLOSE;
- maybe_update_account(NULL, acct, ev, tags, close_output_count, NULL);
+ maybe_update_account(cmd, acct, ev, tags, close_output_count, NULL);
log_chain_event(bkpr, acct,
make_chain_event(ctx, "delayed_to_us",
@@ -867,7 +868,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
blockheight,
'1', 1, '*'));
memset(&txid, '1', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, wal_acct,
make_chain_event(ctx, "anchor",
@@ -884,7 +885,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
blockheight,
'1', 4, '*'));
memset(&txid, '1', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
/* Should be no fees yet */
ofs = list_chain_fees(ctx, bkpr);
@@ -907,7 +908,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
'1', 3, '*'));
memset(&txid, '1', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
db_commit_transaction(db);
/* txid 2222 */
@@ -928,10 +929,10 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
blockheight + 1,
'2', 0, '*'));
memset(&txid, '2', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
CHECK(acct->onchain_resolved_block == 0);
- assert(account_onchain_closeheight(bkpr, NULL, acct) == 0);
+ assert(account_onchain_closeheight(bkpr, cmd, acct) == 0);
CHECK(acct->onchain_resolved_block == 0);
db_commit_transaction(db);
@@ -953,7 +954,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
'4', 0, '*'));
memset(&txid, '4', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
/* txid 3333 */
log_chain_event(bkpr, acct,
@@ -964,7 +965,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
blockheight + 2,
'1', 2, '3'));
- assert(account_onchain_closeheight(bkpr, NULL, acct) == 0);
+ assert(account_onchain_closeheight(bkpr, cmd, acct) == 0);
CHECK(acct->onchain_resolved_block == 0);
log_chain_event(bkpr, acct,
@@ -976,7 +977,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
'3', 0, '*'));
memset(&txid, '3', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, acct,
make_chain_event(ctx, "to_wallet",
@@ -987,7 +988,7 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
'3', 0, '4'));
memset(&txid, '4', sizeof(struct bitcoin_txid));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
db_commit_transaction(db);
@@ -1001,9 +1002,9 @@ static bool test_onchain_fee_chan_close(const tal_t *ctx)
/* Now we update the channel's onchain fees */
CHECK(acct->onchain_resolved_block == 0);
db_begin_transaction(db);
- account_update_closeheight(NULL, acct, account_onchain_closeheight(bkpr, NULL, acct));
+ account_update_closeheight(cmd, acct, account_onchain_closeheight(bkpr, cmd, acct));
CHECK(acct->onchain_resolved_block == blockheight + 2);
- err = update_channel_onchain_fees(ctx, NULL, bkpr, acct);
+ err = update_channel_onchain_fees(ctx, cmd, bkpr, acct);
CHECK_MSG(!err, err);
db_commit_transaction(db);
ofs = account_get_chain_fees(tmpctx, bkpr, acct->name);
@@ -1076,10 +1077,10 @@ static bool test_onchain_fee_chan_open(const tal_t *ctx)
acct2->peer_id = &peer_id;
db_begin_transaction(db);
- account_datastore_set(NULL, wal_acct, "must-create");
- account_datastore_set(NULL, ext_acct, "must-create");
- account_datastore_set(NULL, acct, "must-create");
- account_datastore_set(NULL, acct2, "must-create");
+ account_datastore_set(cmd, wal_acct, "must-create");
+ account_datastore_set(cmd, ext_acct, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
+ account_datastore_set(cmd, acct2, "must-create");
db_commit_transaction(db);
/* Assumption that we rely on later */
@@ -1117,7 +1118,7 @@ static bool test_onchain_fee_chan_open(const tal_t *ctx)
AMOUNT_MSAT(500),
blockheight,
'A', 0, '*'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, acct2,
make_chain_event(ctx, "deposit",
@@ -1126,7 +1127,7 @@ static bool test_onchain_fee_chan_open(const tal_t *ctx)
AMOUNT_MSAT(1000),
blockheight,
'A', 1, '*'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
log_chain_event(bkpr, wal_acct,
make_chain_event(ctx, "deposit",
@@ -1135,9 +1136,9 @@ static bool test_onchain_fee_chan_open(const tal_t *ctx)
AMOUNT_MSAT(2200),
blockheight,
'A', 2, '*'));
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
- maybe_update_onchain_fees(ctx, NULL, bkpr, &txid);
+ maybe_update_onchain_fees(ctx, cmd, bkpr, &txid);
db_commit_transaction(db);
/* Expect: 5 onchain fee records, totaling to 151/150msat ea,
@@ -1196,9 +1197,9 @@ static bool test_channel_rebalances(const tal_t *ctx)
db_begin_transaction(db);
- account_datastore_set(NULL, acct1, "must-create");
- account_datastore_set(NULL, acct2, "must-create");
- account_datastore_set(NULL, acct3, "must-create");
+ account_datastore_set(cmd, acct1, "must-create");
+ account_datastore_set(cmd, acct2, "must-create");
+ account_datastore_set(cmd, acct3, "must-create");
/* Simulate a rebalance of 100msats, w/ a 12msat fee */
ev1 = make_channel_event(ctx, "invoice",
@@ -1224,29 +1225,29 @@ static bool test_channel_rebalances(const tal_t *ctx)
db_commit_transaction(db);
db_begin_transaction(db);
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct1);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct1);
CHECK(tal_count(chan_evs) == 1 && !find_rebalance(bkpr, chan_evs[0]->db_id));
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct2);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct2);
CHECK(tal_count(chan_evs) == 1 && !find_rebalance(bkpr, chan_evs[0]->db_id));
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct3);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct3);
CHECK(tal_count(chan_evs) == 1 && !find_rebalance(bkpr, chan_evs[0]->db_id));
- maybe_record_rebalance(NULL, bkpr, ev2);
+ maybe_record_rebalance(cmd, bkpr, ev2);
CHECK(find_rebalance(bkpr, ev2->db_id) != NULL);
/* Both events should be marked as rebalance */
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct1);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct1);
CHECK(tal_count(chan_evs) == 1 && find_rebalance(bkpr, chan_evs[0]->db_id));
ev1 = chan_evs[0];
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct2);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct2);
CHECK(tal_count(chan_evs) == 1 && find_rebalance(bkpr, chan_evs[0]->db_id));
CHECK(*find_rebalance(bkpr, chan_evs[0]->db_id) == ev1->db_id);
CHECK(*find_rebalance(bkpr, ev1->db_id) == chan_evs[0]->db_id);
/* Third event is not a rebalance though */
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct3);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct3);
CHECK(tal_count(chan_evs) == 1 && !find_rebalance(bkpr, chan_evs[0]->db_id));
/* Did we get an accurate rebalances entry? */
@@ -1279,8 +1280,8 @@ static bool test_channel_event_crud(const tal_t *ctx)
acct2 = new_account(bkpr->accounts, tal_fmt(ctx, ACCOUNT_NAME_WALLET));
acct2->peer_id = &peer_id;
db_begin_transaction(db);
- account_datastore_set(NULL, acct, "must-create");
- account_datastore_set(NULL, acct2, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
+ account_datastore_set(cmd, acct2, "must-create");
db_commit_transaction(db);
ev1 = tal(ctx, struct channel_event);
@@ -1329,7 +1330,7 @@ static bool test_channel_event_crud(const tal_t *ctx)
db_commit_transaction(db);
db_begin_transaction(db);
- chan_evs = account_get_channel_events(ctx, bkpr, NULL, acct);
+ chan_evs = account_get_channel_events(ctx, bkpr, cmd, acct);
db_commit_transaction(db);
CHECK(streq(acct->name, chan_evs[0]->acct_name));
@@ -1358,8 +1359,8 @@ static bool test_chain_event_crud(const tal_t *ctx)
acct2 = new_account(bkpr->accounts, tal_fmt(ctx, ACCOUNT_NAME_WALLET));
acct2->peer_id = &peer_id;
db_begin_transaction(db);
- account_datastore_set(NULL, acct, "must-create");
- account_datastore_set(NULL, acct2, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
+ account_datastore_set(cmd, acct2, "must-create");
db_commit_transaction(db);
/* This event spends the second inserted event */
@@ -1504,8 +1505,8 @@ static bool test_account_balances(const tal_t *ctx)
ok = account_get_balance(bkpr, acct->name, &balance);
CHECK(ok);
- account_datastore_set(NULL, acct, "must-create");
- account_datastore_set(NULL, acct2, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
+ account_datastore_set(cmd, acct2, "must-create");
/* +1000btc */
log_chain_event(bkpr, acct,
@@ -1575,7 +1576,7 @@ static bool test_account_crud(const tal_t *ctx)
CHECK(!acct->is_wallet);
db_begin_transaction(db);
- account_datastore_set(NULL, acct, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
db_commit_transaction(db);
acct_list = list_accounts(ctx, bkpr);
@@ -1586,7 +1587,7 @@ static bool test_account_crud(const tal_t *ctx)
CHECK(acct->is_wallet);
db_begin_transaction(db);
- account_datastore_set(NULL, acct, "must-create");
+ account_datastore_set(cmd, acct, "must-create");
db_commit_transaction(db);
acct_list = list_accounts(ctx, bkpr);
@@ -1622,7 +1623,7 @@ static bool test_account_crud(const tal_t *ctx)
/* should not update the account info */
tags[0] = MVT_PUSHED;
tags[1] = MVT_PENALTY;
- maybe_update_account(NULL, acct, ev1, tags, 0, peer_id);
+ maybe_update_account(cmd, acct, ev1, tags, 0, peer_id);
acct2 = find_account(bkpr, ACCOUNT_NAME_WALLET);
accountseq(acct, acct2);
@@ -1631,7 +1632,7 @@ static bool test_account_crud(const tal_t *ctx)
CHECK(acct->open_event_db_id == NULL);
tags[0] = MVT_CHANNEL_OPEN;
tags[1] = MVT_LEASED;
- maybe_update_account(NULL, acct, ev1, tags, 2, peer_id);
+ maybe_update_account(cmd, acct, ev1, tags, 2, peer_id);
acct2 = find_account(bkpr, ACCOUNT_NAME_WALLET);
accountseq(acct, acct2);
CHECK(acct->leased);
@@ -1642,7 +1643,7 @@ static bool test_account_crud(const tal_t *ctx)
tags[1] = MVT_OPENER;
CHECK(acct->closed_event_db_id == NULL);
CHECK(!acct->we_opened);
- maybe_update_account(NULL, acct, ev1, tags, 0, NULL);
+ maybe_update_account(cmd, acct, ev1, tags, 0, NULL);
acct2 = find_account(bkpr, ACCOUNT_NAME_WALLET);
accountseq(acct, acct2);
CHECK(acct->closed_event_db_id != NULL);
@@ -1660,6 +1661,8 @@ int main(int argc, char *argv[])
common_setup(argv[0]);
if (HAVE_SQLITE3) {
+ /* UBSan insists cmd isn't NULL */
+ cmd = tal(tmpctx, struct command);
ok &= test_account_crud(tmpctx);
ok &= test_channel_event_crud(tmpctx);
ok &= test_chain_event_crud(tmpctx);
Risk score
Our methodology →Why this scored 15/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.