From fbbc5899e433c513256fcf25b27ee2afe0eadb98 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 27 Jul 2018 12:57:02 +0200 Subject: [PATCH] invoices: Make the invoice_details more idiomatic This seems like a premature optimization: it tried to cut down the number of allocations by reusing the same `struct invoice_details` while iterating through a number of results. But this sidesteps the checks by `valgrind` and we'd miss a missing field that was set by the previous iteration. Reported-by: @rustyrussell Signed-off-by: Christian Decker <@cdecker> --- lightningd/invoice.c | 54 ++++++++++++++++++++-------------------- lightningd/peer_htlcs.c | 14 +++++------ wallet/invoices.c | 37 +++++++++++++-------------- wallet/invoices.h | 18 ++++++-------- wallet/test/run-wallet.c | 14 +++++------ wallet/wallet.c | 20 +++++++-------- wallet/wallet.h | 19 ++++++-------- 7 files changed, 84 insertions(+), 92 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index cd8baf891..b2494b041 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -60,11 +60,11 @@ static void json_add_invoice(struct json_result *response, static void tell_waiter(struct command *cmd, const struct invoice *inv) { struct json_result *response = new_json_result(cmd); - struct invoice_details details; + const struct invoice_details *details; - wallet_invoice_details(cmd, cmd->ld->wallet, *inv, &details); - json_add_invoice(response, &details); - if (details.state == PAID) + details = wallet_invoice_details(cmd, cmd->ld->wallet, *inv); + json_add_invoice(response, details); + if (details->state == PAID) command_success(cmd, response); else { /* FIXME: -2 should be a constant in jsonrpc_errors.h. */ @@ -153,7 +153,7 @@ static void json_invoice(struct command *cmd, const char *buffer, const jsmntok_t *params) { struct invoice invoice; - struct invoice_details details; + const struct invoice_details *details; const jsmntok_t *msatoshi, *label, *desctok, *fallbacks; const jsmntok_t *preimagetok; u64 *msatoshi_val; @@ -320,23 +320,23 @@ static void json_invoice(struct command *cmd, } /* Get details */ - wallet_invoice_details(cmd, cmd->ld->wallet, invoice, &details); + details = wallet_invoice_details(cmd, cmd->ld->wallet, invoice); json_object_start(response, NULL); - json_add_hex(response, "payment_hash", - &details.rhash, sizeof(details.rhash)); - json_add_u64(response, "expires_at", details.expiry_time); - json_add_string(response, "bolt11", details.bolt11); + json_add_hex(response, "payment_hash", details->rhash.u.u8, + sizeof(details->rhash)); + json_add_u64(response, "expires_at", details->expiry_time); + json_add_string(response, "bolt11", details->bolt11); json_object_end(response); command_success(cmd, response); } static const struct json_command invoice_command = { - "invoice", - json_invoice, - "Create an invoice for {msatoshi} with {label} and {description} with optional {expiry} seconds (default 1 hour) and optional {preimage} (default autogenerated)" -}; + "invoice", json_invoice, "Create an invoice for {msatoshi} with {label} " + "and {description} with optional {expiry} seconds " + "(default 1 hour) and optional {preimage} " + "(default autogenerated)"}; AUTODATA(json_command, &invoice_command); static void json_add_invoices(struct json_result *response, @@ -344,23 +344,22 @@ static void json_add_invoices(struct json_result *response, const struct json_escaped *label) { struct invoice_iterator it; - struct invoice_details details; + const struct invoice_details *details; /* Don't iterate entire db if we're just after one. */ if (label) { struct invoice invoice; if (wallet_invoice_find_by_label(wallet, &invoice, label)) { - wallet_invoice_details(response, wallet, invoice, - &details); - json_add_invoice(response, &details); + details = wallet_invoice_details(response, wallet, invoice); + json_add_invoice(response, details); } return; } memset(&it, 0, sizeof(it)); while (wallet_invoice_iterate(wallet, &it)) { - wallet_invoice_iterator_deref(response, wallet, &it, &details); - json_add_invoice(response, &details); + details = wallet_invoice_iterator_deref(response, wallet, &it); + json_add_invoice(response, details); } } @@ -408,7 +407,7 @@ static void json_delinvoice(struct command *cmd, const char *buffer, const jsmntok_t *params) { struct invoice i; - struct invoice_details details; + const struct invoice_details *details; const jsmntok_t *labeltok, *statustok; struct json_result *response = new_json_result(cmd); const char *status, *actual_status; @@ -433,13 +432,14 @@ static void json_delinvoice(struct command *cmd, command_fail(cmd, LIGHTNINGD, "Unknown invoice"); return; } - wallet_invoice_details(cmd, cmd->ld->wallet, i, &details); + + details = wallet_invoice_details(cmd, cmd->ld->wallet, i); status = tal_strndup(cmd, buffer + statustok->start, statustok->end - statustok->start); /* This is time-sensitive, so only call once; otherwise error msg * might not make sense if it changed! */ - actual_status = invoice_status_str(&details); + actual_status = invoice_status_str(details); if (!streq(actual_status, status)) { command_fail(cmd, LIGHTNINGD, "Invoice status is %s not %s", actual_status, status); @@ -448,7 +448,7 @@ static void json_delinvoice(struct command *cmd, /* Get invoice details before attempting to delete, as * otherwise the invoice will be freed. */ - json_add_invoice(response, &details); + json_add_invoice(response, details); if (!wallet_invoice_delete(wallet, i)) { log_broken(cmd->ld->log, @@ -562,7 +562,7 @@ static void json_waitinvoice(struct command *cmd, const char *buffer, const jsmntok_t *params) { struct invoice i; - struct invoice_details details; + const struct invoice_details *details; struct wallet *wallet = cmd->ld->wallet; const jsmntok_t *labeltok; struct json_escaped *label; @@ -586,10 +586,10 @@ static void json_waitinvoice(struct command *cmd, command_fail(cmd, LIGHTNINGD, "Label not found"); return; } - wallet_invoice_details(cmd, cmd->ld->wallet, i, &details); + details = wallet_invoice_details(cmd, cmd->ld->wallet, i); /* If paid or expired return immediately */ - if (details.state == PAID || details.state == EXPIRED) { + if (details->state == PAID || details->state == EXPIRED) { tell_waiter(cmd, &i); return; } else { diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 9a9d6bfb6..519d7ad8b 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -249,7 +249,7 @@ static void handle_localpay(struct htlc_in *hin, { enum onion_type failcode; struct invoice invoice; - struct invoice_details details; + const struct invoice_details *details; struct lightningd *ld = hin->key.channel->peer->ld; /* BOLT #4: @@ -282,7 +282,7 @@ static void handle_localpay(struct htlc_in *hin, failcode = WIRE_UNKNOWN_PAYMENT_HASH; goto fail; } - wallet_invoice_details(tmpctx, ld->wallet, invoice, &details); + details = wallet_invoice_details(tmpctx, ld->wallet, invoice); /* BOLT #4: * @@ -298,10 +298,10 @@ static void handle_localpay(struct htlc_in *hin, * leakage by altering the amount while not allowing for * accidental gross overpayment. */ - if (details.msatoshi != NULL && hin->msatoshi < *details.msatoshi) { + if (details->msatoshi != NULL && hin->msatoshi < *details->msatoshi) { failcode = WIRE_INCORRECT_PAYMENT_AMOUNT; goto fail; - } else if (details.msatoshi != NULL && hin->msatoshi > *details.msatoshi * 2) { + } else if (details->msatoshi != NULL && hin->msatoshi > *details->msatoshi * 2) { failcode = WIRE_INCORRECT_PAYMENT_AMOUNT; goto fail; } @@ -324,10 +324,10 @@ static void handle_localpay(struct htlc_in *hin, } log_info(ld->log, "Resolving invoice '%s' with HTLC %"PRIu64, - details.label->s, hin->key.id); + details->label->s, hin->key.id); log_debug(ld->log, "%s: Actual amount %"PRIu64"msat, HTLC expiry %u", - details.label->s, hin->msatoshi, cltv_expiry); - fulfill_htlc(hin, &details.r); + details->label->s, hin->msatoshi, cltv_expiry); + fulfill_htlc(hin, &details->r); wallet_invoice_resolve(ld->wallet, invoice, hin->msatoshi); return; diff --git a/wallet/invoices.c b/wallet/invoices.c index 7cd1565d1..96889e580 100644 --- a/wallet/invoices.c +++ b/wallet/invoices.c @@ -87,20 +87,20 @@ trigger_invoice_waiter_expire_or_delete(struct invoices *invoices, } } -static void wallet_stmt2invoice_details(const tal_t *ctx, - sqlite3_stmt *stmt, - struct invoice_details *dtl) +static struct invoice_details *wallet_stmt2invoice_details(const tal_t *ctx, + sqlite3_stmt *stmt) { + struct invoice_details *dtl = tal(ctx, struct invoice_details); dtl->state = sqlite3_column_int(stmt, 0); sqlite3_column_preimage(stmt, 1, &dtl->r); sqlite3_column_sha256(stmt, 2, &dtl->rhash); - dtl->label = sqlite3_column_json_escaped(ctx, stmt, 3); + dtl->label = sqlite3_column_json_escaped(dtl, stmt, 3); if (sqlite3_column_type(stmt, 4) != SQLITE_NULL) { - dtl->msatoshi = tal(ctx, u64); + dtl->msatoshi = tal(dtl, u64); *dtl->msatoshi = sqlite3_column_int64(stmt, 4); } else { dtl->msatoshi = NULL; @@ -114,14 +114,16 @@ static void wallet_stmt2invoice_details(const tal_t *ctx, dtl->paid_timestamp = sqlite3_column_int64(stmt, 8); } - dtl->bolt11 = tal_strndup(ctx, sqlite3_column_blob(stmt, 9), + dtl->bolt11 = tal_strndup(dtl, sqlite3_column_blob(stmt, 9), sqlite3_column_bytes(stmt, 9)); if (sqlite3_column_type(stmt, 10) != SQLITE_NULL) dtl->description = tal_strdup( - ctx, (const char *)sqlite3_column_text(stmt, 10)); + dtl, (const char *)sqlite3_column_text(stmt, 10)); else dtl->description = NULL; + + return dtl; } struct invoices *invoices_new(const tal_t *ctx, @@ -492,13 +494,12 @@ bool invoices_iterate(struct invoices *invoices, return true; } } -void invoices_iterator_deref(const tal_t *ctx, - struct invoices *invoices UNUSED, - const struct invoice_iterator *it, - struct invoice_details *details) +const struct invoice_details * +invoices_iterator_deref(const tal_t *ctx, struct invoices *invoices UNUSED, + const struct invoice_iterator *it) { assert(it->p); - wallet_stmt2invoice_details(ctx, (sqlite3_stmt*) it->p, details); + return wallet_stmt2invoice_details(ctx, (sqlite3_stmt*) it->p); } static s64 get_next_pay_index(struct db *db) @@ -638,13 +639,13 @@ void invoices_waitone(const tal_t *ctx, false, invoice.id, cb, cbarg); } -void invoices_get_details(const tal_t *ctx, - struct invoices *invoices, - struct invoice invoice, - struct invoice_details *dtl) +const struct invoice_details *invoices_get_details(const tal_t *ctx, + struct invoices *invoices, + struct invoice invoice) { sqlite3_stmt *stmt; int result; + struct invoice_details *details; stmt = db_prepare(invoices->db, "SELECT state, payment_key, payment_hash" @@ -657,7 +658,7 @@ void invoices_get_details(const tal_t *ctx, result = sqlite3_step(stmt); assert(result == SQLITE_ROW); - wallet_stmt2invoice_details(ctx, stmt, dtl); - + details = wallet_stmt2invoice_details(ctx, stmt); db_stmt_done(stmt); + return details; } diff --git a/wallet/invoices.h b/wallet/invoices.h index e41ddd555..00f8fa874 100644 --- a/wallet/invoices.h +++ b/wallet/invoices.h @@ -169,13 +169,12 @@ bool invoices_iterate(struct invoices *invoices, * @ctx - the owner of the label and msatoshi fields returned. * @wallet - the wallet whose invoices are to be iterated over. * @iterator - the iterator object to use. - * @details - pointer to details object to load. + * @return The invoice details allocated off of `ctx` * */ -void invoices_iterator_deref(const tal_t *ctx, - struct invoices *invoices, - const struct invoice_iterator *it, - struct invoice_details *details); +const struct invoice_details *invoices_iterator_deref( + const tal_t *ctx, struct invoices *invoices, + const struct invoice_iterator *it); /** * invoices_resolve - Mark an invoice as paid @@ -239,11 +238,10 @@ void invoices_waitone(const tal_t *ctx, * @ctx - the owner of the label and msatoshi fields returned. * @invoices - the invoice handler, * @invoice - the invoice to get details on. - * @details - pointer to details object to load. + * @return pointer to the invoice details allocated off of `ctx`. */ -void invoices_get_details(const tal_t *ctx, - struct invoices *invoices, - struct invoice invoice, - struct invoice_details *details); +const struct invoice_details *invoices_get_details(const tal_t *ctx, + struct invoices *invoices, + struct invoice invoice); #endif /* LIGHTNING_WALLET_INVOICES_H */ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index f9f3cb611..858dfd518 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -137,20 +137,18 @@ bool invoices_find_unpaid(struct invoices *invoices UNNEEDED, const struct sha256 *rhash UNNEEDED) { fprintf(stderr, "invoices_find_unpaid called!\n"); abort(); } /* Generated stub for invoices_get_details */ -void invoices_get_details(const tal_t *ctx UNNEEDED, - struct invoices *invoices UNNEEDED, - struct invoice invoice UNNEEDED, - struct invoice_details *details UNNEEDED) +const struct invoice_details *invoices_get_details(const tal_t *ctx UNNEEDED, + struct invoices *invoices UNNEEDED, + struct invoice invoice UNNEEDED) { fprintf(stderr, "invoices_get_details called!\n"); abort(); } /* Generated stub for invoices_iterate */ bool invoices_iterate(struct invoices *invoices UNNEEDED, struct invoice_iterator *it UNNEEDED) { fprintf(stderr, "invoices_iterate called!\n"); abort(); } /* Generated stub for invoices_iterator_deref */ -void invoices_iterator_deref(const tal_t *ctx UNNEEDED, - struct invoices *invoices UNNEEDED, - const struct invoice_iterator *it UNNEEDED, - struct invoice_details *details UNNEEDED) +const struct invoice_details *invoices_iterator_deref( + const tal_t *ctx UNNEEDED, struct invoices *invoices UNNEEDED, + const struct invoice_iterator *it UNNEEDED) { fprintf(stderr, "invoices_iterator_deref called!\n"); abort(); } /* Generated stub for invoices_load */ bool invoices_load(struct invoices *invoices UNNEEDED) diff --git a/wallet/wallet.c b/wallet/wallet.c index 9c563c406..079d5c96c 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1444,12 +1444,11 @@ bool wallet_invoice_iterate(struct wallet *wallet, { return invoices_iterate(wallet->invoices, it); } -void wallet_invoice_iterator_deref(const tal_t *ctx, - struct wallet *wallet, - const struct invoice_iterator *it, - struct invoice_details *details) +const struct invoice_details * +wallet_invoice_iterator_deref(const tal_t *ctx, struct wallet *wallet, + const struct invoice_iterator *it) { - return invoices_iterator_deref(ctx, wallet->invoices, it, details); + return invoices_iterator_deref(ctx, wallet->invoices, it); } void wallet_invoice_resolve(struct wallet *wallet, struct invoice invoice, @@ -1473,15 +1472,14 @@ void wallet_invoice_waitone(const tal_t *ctx, { invoices_waitone(ctx, wallet->invoices, invoice, cb, cbarg); } -void wallet_invoice_details(const tal_t *ctx, - struct wallet *wallet, - struct invoice invoice, - struct invoice_details *details) + +const struct invoice_details *wallet_invoice_details(const tal_t *ctx, + struct wallet *wallet, + struct invoice invoice) { - invoices_get_details(ctx, wallet->invoices, invoice, details); + return invoices_get_details(ctx, wallet->invoices, invoice); } - struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, struct channel *chan) { diff --git a/wallet/wallet.h b/wallet/wallet.h index 6ac71bd00..0e556ba06 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -605,13 +605,11 @@ bool wallet_invoice_iterate(struct wallet *wallet, * @ctx - the owner of the label and msatoshi fields returned. * @wallet - the wallet whose invoices are to be iterated over. * @iterator - the iterator object to use. - * @details - pointer to details object to load. - * + * @return pointer to the invoice details allocated off of `ctx`. */ -void wallet_invoice_iterator_deref(const tal_t *ctx, - struct wallet *wallet, - const struct invoice_iterator *it, - struct invoice_details *details); +const struct invoice_details * +wallet_invoice_iterator_deref(const tal_t *ctx, struct wallet *wallet, + const struct invoice_iterator *it); /** * wallet_invoice_resolve - Mark an invoice as paid @@ -676,12 +674,11 @@ void wallet_invoice_waitone(const tal_t *ctx, * @ctx - the owner of the label and msatoshi fields returned. * @wallet - the wallet to query. * @invoice - the invoice to get details on. - * @details - pointer to details object to load. + * @return pointer to the invoice details allocated off of `ctx`. */ -void wallet_invoice_details(const tal_t *ctx, - struct wallet *wallet, - struct invoice invoice, - struct invoice_details *details); +const struct invoice_details *wallet_invoice_details(const tal_t *ctx, + struct wallet *wallet, + struct invoice invoice); /** * wallet_htlc_stubs - Retrieve HTLC stubs for the given channel