From 91a22dc496f710e7bc36ded004e418ed4437c935 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 29 Jan 2018 11:04:28 +1030 Subject: [PATCH] jsonprc: make json_get_params() fail the command, for better error reporting. We move it into jsonrpc where it belongs, and make it fail the command. This means it can tell us exactly what was wrong. Signed-off-by: Rusty Russell --- common/json.c | 84 -------------------------- common/json.h | 9 --- lightningd/chaintopology.c | 6 +- lightningd/dev_ping.c | 3 +- lightningd/gossip_control.c | 9 +-- lightningd/invoice.c | 18 ++---- lightningd/jsonrpc.c | 117 ++++++++++++++++++++++++++++++++---- lightningd/jsonrpc.h | 10 +++ lightningd/options.c | 3 +- lightningd/pay.c | 9 +-- lightningd/peer_control.c | 21 +++---- wallet/walletrpc.c | 3 +- 12 files changed, 138 insertions(+), 154 deletions(-) diff --git a/common/json.c b/common/json.c index 8a70ca23e..d4a3908f5 100644 --- a/common/json.c +++ b/common/json.c @@ -225,90 +225,6 @@ const jsmntok_t *json_delve(const char *buffer, return tok; } -bool json_get_params(const char *buffer, const jsmntok_t param[], ...) -{ - va_list ap; - const char **names; - size_t num_names; - /* Uninitialized warnings on p and end */ - const jsmntok_t **tokptr, *p = NULL, *end = NULL; - - if (param->type == JSMN_ARRAY) { - if (param->size == 0) - p = NULL; - else - p = param + 1; - end = json_next(param); - } else if (param->type != JSMN_OBJECT) - return false; - - num_names = 0; - names = tal_arr(NULL, const char *, num_names + 1); - va_start(ap, param); - while ((names[num_names] = va_arg(ap, const char *)) != NULL) { - tokptr = va_arg(ap, const jsmntok_t **); - bool compulsory = true; - if (names[num_names][0] == '?') { - names[num_names]++; - compulsory = false; - } - if (param->type == JSMN_ARRAY) { - *tokptr = p; - if (p) { - p = json_next(p); - if (p == end) - p = NULL; - } - } else { - *tokptr = json_get_member(buffer, param, - names[num_names]); - } - /* Convert 'null' to NULL */ - if (*tokptr - && (*tokptr)->type == JSMN_PRIMITIVE - && buffer[(*tokptr)->start] == 'n') { - *tokptr = NULL; - } - if (compulsory && !*tokptr) { - va_end(ap); - tal_free(names); - return false; - } - num_names++; - tal_resize(&names, num_names + 1); - } - - va_end(ap); - - /* Now make sure there aren't any params which aren't valid */ - if (param->type == JSMN_ARRAY) { - if (param->size > num_names) { - tal_free(names); - return false; - } - } else { - const jsmntok_t *t; - - end = json_next(param); - - /* Find each parameter among the valid names */ - for (t = param + 1; t < end; t = json_next(t+1)) { - bool found = false; - for (size_t i = 0; i < num_names; i++) { - if (json_tok_streq(buffer, t, names[i])) - found = true; - } - if (!found) { - tal_free(names); - return false; - } - } - } - - tal_free(names); - return true; -} - static bool strange_chars(const char *str, size_t len) { for (size_t i = 0; i < len; i++) { diff --git a/common/json.h b/common/json.h index 796f387fd..6fc1cdde3 100644 --- a/common/json.h +++ b/common/json.h @@ -46,15 +46,6 @@ bool json_tok_is_null(const char *buffer, const jsmntok_t *tok); /* Returns next token with same parent. */ const jsmntok_t *json_next(const jsmntok_t *tok); -/* Get the parameters (by position or name). Followed by triples of - * of const char *name, const jsmntok_t **ret_ptr, then NULL. - * - * If name starts with '?' it is optional (and will be set to NULL - * if it's a literal 'null' or not present). - * Otherwise false is returned. - */ -bool json_get_params(const char *buffer, const jsmntok_t param[], ...); - /* Get top-level member. */ const jsmntok_t *json_get_member(const char *buffer, const jsmntok_t tok[], const char *label); diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 96df5fc16..c828d30c2 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -556,10 +556,9 @@ void json_dev_broadcast(struct command *cmd, jsmntok_t *enabletok; bool enable; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "enable", &enabletok, NULL)) { - command_fail(cmd, "Need enable"); return; } @@ -606,12 +605,11 @@ static void json_dev_setfees(struct command *cmd, struct chain_topology *topo = cmd->ld->topology; struct json_result *response; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?immediate", &ratetok[FEERATE_IMMEDIATE], "?normal", &ratetok[FEERATE_NORMAL], "?slow", &ratetok[FEERATE_SLOW], NULL)) { - command_fail(cmd, "Bad parameters"); return; } diff --git a/lightningd/dev_ping.c b/lightningd/dev_ping.c index c73b88515..e3a20d8d0 100644 --- a/lightningd/dev_ping.c +++ b/lightningd/dev_ping.c @@ -45,12 +45,11 @@ static void json_dev_ping(struct command *cmd, struct pubkey id; struct subd *owner; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &idtok, "len", &lentok, "pongbytes", &pongbytestok, NULL)) { - command_fail(cmd, "Need id, len and pongbytes"); return; } diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index f5a256f9a..9dd3228f3 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -235,10 +235,9 @@ static void json_listnodes(struct command *cmd, const char *buffer, jsmntok_t *idtok = NULL; struct pubkey *id = NULL; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?id", &idtok, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } @@ -302,13 +301,12 @@ static void json_getroute(struct command *cmd, const char *buffer, const jsmntok double riskfactor; struct lightningd *ld = cmd->ld; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &idtok, "msatoshi", &msatoshitok, "riskfactor", &riskfactortok, "?cltv", &cltvtok, NULL)) { - command_fail(cmd, "Need id, msatoshi and riskfactor"); return; } @@ -396,10 +394,9 @@ static void json_listchannels(struct command *cmd, const char *buffer, jsmntok_t *idtok; struct short_channel_id *id = NULL; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?short_channel_id", &idtok, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 318efa74c..225e41eae 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -108,13 +108,12 @@ static void json_invoice(struct command *cmd, char *b11enc; u64 expiry = 3600; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "msatoshi", &msatoshi, "label", &label, "description", &desc, "?expiry", &exp, NULL)) { - command_fail(cmd, "Need {msatoshi}, {label} and {description}"); return; } @@ -228,10 +227,9 @@ static void json_listinvoice_internal(struct command *cmd, struct json_result *response = new_json_result(cmd); struct wallet *wallet = cmd->ld->wallet; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?label", &label, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } @@ -284,11 +282,10 @@ static void json_delinvoice(struct command *cmd, const char *label, *status, *actual_status; struct wallet *wallet = cmd->ld->wallet; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "label", &labeltok, "status", &statustok, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } @@ -340,10 +337,9 @@ static void json_waitanyinvoice(struct command *cmd, u64 pay_index; struct wallet *wallet = cmd->ld->wallet; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?lastpay_index", &pay_indextok, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } @@ -389,8 +385,7 @@ static void json_waitinvoice(struct command *cmd, jsmntok_t *labeltok; const char *label = NULL; - if (!json_get_params(buffer, params, "label", &labeltok, NULL)) { - command_fail(cmd, "Missing {label}"); + if (!json_get_params(cmd, buffer, params, "label", &labeltok, NULL)) { return; } @@ -427,11 +422,10 @@ static void json_decodepay(struct command *cmd, struct json_result *response; char *str, *desc, *fail; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "bolt11", &bolt11tok, "?description", &desctok, NULL)) { - command_fail(cmd, "Need bolt11 string"); return; } diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index fb7854387..57e260e34 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -155,8 +155,7 @@ static void json_getlog(struct command *cmd, struct log_book *lr = cmd->ld->log_book; jsmntok_t *level; - if (!json_get_params(buffer, params, "?level", &level, NULL)) { - command_fail(cmd, "Invalid arguments"); + if (!json_get_params(cmd, buffer, params, "?level", &level, NULL)) { return; } @@ -206,10 +205,9 @@ static void json_rhash(struct command *cmd, jsmntok_t *secrettok; struct sha256 secret; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "secret", &secrettok, NULL)) { - command_fail(cmd, "Need secret"); return; } @@ -298,8 +296,7 @@ static void json_help(struct command *cmd, struct json_command **cmdlist = get_cmdlist(); jsmntok_t *cmdtok; - if (!json_get_params(buffer, params, "?command", &cmdtok, NULL)) { - command_fail(cmd, "Invalid arguments"); + if (!json_get_params(cmd, buffer, params, "?command", &cmdtok, NULL)) { return; } @@ -626,13 +623,6 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[]) return; } - if (params->type != JSMN_ARRAY && params->type != JSMN_OBJECT) { - command_fail_detailed(jcon->current, - JSONRPC2_INVALID_PARAMS, NULL, - "Expected array or object for params"); - return; - } - db_begin_transaction(jcon->ld->wallet->db); cmd->dispatch(jcon->current, jcon->buffer, params); db_commit_transaction(jcon->ld->wallet->db); @@ -642,6 +632,107 @@ static void parse_request(struct json_connection *jcon, const jsmntok_t tok[]) assert(jcon->current->pending); } +bool json_get_params(struct command *cmd, + const char *buffer, const jsmntok_t param[], ...) +{ + va_list ap; + const char **names; + size_t num_names; + /* Uninitialized warnings on p and end */ + const jsmntok_t **tokptr, *p = NULL, *end = NULL; + + if (param->type == JSMN_ARRAY) { + if (param->size == 0) + p = NULL; + else + p = param + 1; + end = json_next(param); + } else if (param->type != JSMN_OBJECT) { + command_fail_detailed(cmd, JSONRPC2_INVALID_PARAMS, NULL, + "Expected array or object for params"); + return false; + } + + num_names = 0; + names = tal_arr(cmd, const char *, num_names + 1); + va_start(ap, param); + while ((names[num_names] = va_arg(ap, const char *)) != NULL) { + tokptr = va_arg(ap, const jsmntok_t **); + bool compulsory = true; + if (names[num_names][0] == '?') { + names[num_names]++; + compulsory = false; + } + if (param->type == JSMN_ARRAY) { + *tokptr = p; + if (p) { + p = json_next(p); + if (p == end) + p = NULL; + } + } else { + *tokptr = json_get_member(buffer, param, + names[num_names]); + } + /* Convert 'null' to NULL */ + if (*tokptr + && (*tokptr)->type == JSMN_PRIMITIVE + && buffer[(*tokptr)->start] == 'n') { + *tokptr = NULL; + } + if (compulsory && !*tokptr) { + va_end(ap); + command_fail_detailed(cmd, JSONRPC2_INVALID_PARAMS, NULL, + "Missing '%s' parameter", + names[num_names]); + tal_free(names); + return false; + } + num_names++; + tal_resize(&names, num_names + 1); + } + + va_end(ap); + + /* Now make sure there aren't any params which aren't valid */ + if (param->type == JSMN_ARRAY) { + if (param->size > num_names) { + tal_free(names); + command_fail_detailed(cmd, JSONRPC2_INVALID_PARAMS, NULL, + "Too many parameters:" + " got %u, expected %zu", + param->size, num_names); + return false; + } + } else { + const jsmntok_t *t; + + end = json_next(param); + + /* Find each parameter among the valid names */ + for (t = param + 1; t < end; t = json_next(t+1)) { + bool found = false; + for (size_t i = 0; i < num_names; i++) { + if (json_tok_streq(buffer, t, names[i])) + found = true; + } + if (!found) { + tal_free(names); + command_fail_detailed(cmd, + JSONRPC2_INVALID_PARAMS, + NULL, + "Unknown parameter '%.*s'", + t->end - t->start, + buffer + t->start); + return false; + } + } + } + + tal_free(names); + return true; +} + static struct io_plan *write_json(struct io_conn *conn, struct json_connection *jcon) { diff --git a/lightningd/jsonrpc.h b/lightningd/jsonrpc.h index e4ba0439b..90f105cd8 100644 --- a/lightningd/jsonrpc.h +++ b/lightningd/jsonrpc.h @@ -56,6 +56,16 @@ struct json_command { const char *verbose; }; +/* Get the parameters (by position or name). Followed by triples of + * of const char *name, const jsmntok_t **ret_ptr, then NULL. + * + * If name starts with '?' it is optional (and will be set to NULL + * if it's a literal 'null' or not present). + * Otherwise false is returned, and command_fail already called. + */ +bool json_get_params(struct command *cmd, + const char *buffer, const jsmntok_t param[], ...); + struct json_result *null_response(const tal_t *ctx); void command_success(struct command *cmd, struct json_result *response); void PRINTF_FMT(2, 3) command_fail(struct command *cmd, const char *fmt, ...); diff --git a/lightningd/options.c b/lightningd/options.c index 433ec9b67..57a75c849 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -774,8 +774,7 @@ static void json_listconfigs(struct command *cmd, jsmntok_t *configtok; bool found = false; - if (!json_get_params(buffer, params, "?config", &configtok, NULL)) { - command_fail(cmd, "Invalid arguments"); + if (!json_get_params(cmd, buffer, params, "?config", &configtok, NULL)) { return; } diff --git a/lightningd/pay.c b/lightningd/pay.c index 616bd6740..5a6a9c7ce 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -432,11 +432,10 @@ static void json_sendpay(struct command *cmd, struct sha256 rhash; struct route_hop *route; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "route", &routetok, "rhash", &rhashtok, NULL)) { - command_fail(cmd, "Need route and rhash"); return; } @@ -553,13 +552,12 @@ static void json_pay(struct command *cmd, char *fail, *b11str, *desc; u8 *req; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "bolt11", &bolt11tok, "?msatoshi", &msatoshitok, "?description", &desctok, "?riskfactor", &riskfactortok, NULL)) { - command_fail(cmd, "Need bolt11 string"); return; } @@ -632,11 +630,10 @@ static void json_listpayments(struct command *cmd, const char *buffer, jsmntok_t *bolt11tok, *rhashtok; struct sha256 *rhash = NULL; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?bolt11", &bolt11tok, "?payment_hash", &rhashtok, NULL)) { - command_fail(cmd, "Invalid parameters"); return; } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 073c1b3c2..c5920b242 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -742,12 +742,11 @@ static void json_connect(struct command *cmd, struct wireaddr addr; u8 *msg; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &idtok, "?host", &hosttok, "?port", &porttok, NULL)) { - command_fail(cmd, "Need id to connect"); return; } @@ -949,11 +948,10 @@ static void json_listpeers(struct command *cmd, gpa->cmd = cmd; gpa->specific_id = NULL; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "?id", &idtok, "?level", &leveltok, NULL)) { - command_fail(cmd, "Invalid arguments"); return; } @@ -2673,11 +2671,10 @@ static void json_fund_channel(struct command *cmd, struct funding_channel *fc = tal(cmd, struct funding_channel); u8 *msg; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &peertok, "satoshi", &satoshitok, NULL)) { - command_fail(cmd, "Need id and satoshi"); return; } @@ -2732,10 +2729,9 @@ static void json_close(struct command *cmd, jsmntok_t *peertok; struct peer *peer; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &peertok, NULL)) { - command_fail(cmd, "Need id"); return; } @@ -2849,10 +2845,9 @@ static void json_sign_last_tx(struct command *cmd, struct json_result *response = new_json_result(cmd); u8 *linear; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &peertok, NULL)) { - command_fail(cmd, "Need id"); return; } @@ -2891,10 +2886,9 @@ static void json_dev_fail(struct command *cmd, jsmntok_t *peertok; struct peer *peer; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &peertok, NULL)) { - command_fail(cmd, "Need id"); return; } @@ -2930,10 +2924,9 @@ static void json_dev_reenable_commit(struct command *cmd, struct peer *peer; u8 *msg; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "id", &peertok, NULL)) { - command_fail(cmd, "Need id"); return; } diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index e052c2330..3e63afdd9 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -199,11 +199,10 @@ static void json_withdraw(struct command *cmd, struct bitcoin_tx *tx; bool withdraw_all = false; - if (!json_get_params(buffer, params, + if (!json_get_params(cmd, buffer, params, "destination", &desttok, "satoshi", &sattok, NULL)) { - command_fail(cmd, "Need destination and satoshi."); return; }