sphinx: Return the error in parse_onionpacket

As suggested by @niftynei here: https://github.com/ElementsProject/lightning/pull/3260#discussion_r347543999

Suggested-by: Lisa Neigut <@niftynei>
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>

channeld/channeld.c

@@ -584,25 +584,24 @@ static struct secret *get_shared_secret(const tal_t *ctx,
 					enum onion_type *why_bad,
 					struct sha256 *next_onion_sha)
 {
-	struct onionpacket *op;
+	struct onionpacket op;
 	struct secret *secret = tal(ctx, struct secret);
 	const u8 *msg;
 	struct route_step *rs;
 
 	/* We unwrap the onion now. */
-	op = parse_onionpacket(tmpctx, htlc->routing, TOTAL_PACKET_SIZE,
+	*why_bad = parse_onionpacket(htlc->routing, TOTAL_PACKET_SIZE, &op);
-			       why_bad);
+	if (*why_bad != 0)
-	if (!op)
 		return tal_free(secret);
 
 	/* Because wire takes struct pubkey. */
-	msg = hsm_req(tmpctx, towire_hsm_ecdh_req(tmpctx, &op->ephemeralkey));
+	msg = hsm_req(tmpctx, towire_hsm_ecdh_req(tmpctx, &op.ephemeralkey));
 	if (!fromwire_hsm_ecdh_resp(msg, secret))
 		status_failed(STATUS_FAIL_HSM_IO, "Reading ecdh response");
 
 	/* We make sure we can parse onion packet, so we know if shared secret
 	 * is actually valid (this checks hmac). */
-	rs = process_onionpacket(tmpctx, op, secret->data,
+	rs = process_onionpacket(tmpctx, &op, secret->data,
 				 htlc->rhash.u.u8,
 				 sizeof(htlc->rhash));
 	if (!rs) {

common/sphinx.c

@@ -136,36 +136,30 @@ u8 *serialize_onionpacket(
 	return dst;
 }
 
-struct onionpacket *parse_onionpacket(const tal_t *ctx,
+enum onion_type parse_onionpacket(const u8 *src,
-				      const void *src,
 				  const size_t srclen,
-				      enum onion_type *why_bad)
+				  struct onionpacket *dest)
 {
-	struct onionpacket *m;
 	int p = 0;
 	u8 rawEphemeralkey[PUBKEY_CMPR_LEN];
 
 	assert(srclen == TOTAL_PACKET_SIZE);
 
-	m = talz(ctx, struct onionpacket);
+	read_buffer(&dest->version, src, 1, &p);
-
+	if (dest->version != 0x00) {
-	read_buffer(&m->version, src, 1, &p);
-	if (m->version != 0x00) {
 		// FIXME add logging
-		*why_bad = WIRE_INVALID_ONION_VERSION;
+		return WIRE_INVALID_ONION_VERSION;
-		return tal_free(m);
 	}
 	read_buffer(rawEphemeralkey, src, sizeof(rawEphemeralkey), &p);
 
 	if (!pubkey_from_der(rawEphemeralkey, sizeof(rawEphemeralkey),
-			     &m->ephemeralkey)) {
+			     &dest->ephemeralkey)) {
-		*why_bad = WIRE_INVALID_ONION_KEY;
+		return WIRE_INVALID_ONION_KEY;
-		return tal_free(m);
 	}
 
-	read_buffer(&m->routinginfo, src, ROUTING_INFO_SIZE, &p);
+	read_buffer(&dest->routinginfo, src, ROUTING_INFO_SIZE, &p);
-	read_buffer(&m->mac, src, HMAC_SIZE, &p);
+	read_buffer(&dest->mac, src, HMAC_SIZE, &p);
-	return m;
+	return 0;
 }
 
 static void xorbytes(uint8_t *d, const uint8_t *a, const uint8_t *b, size_t len)

common/sphinx.h

@@ -148,15 +148,13 @@ u8 *serialize_onionpacket(
 /**
  * parse_onionpacket - Parse an onionpacket from a buffer.
  *
- * @ctx: tal context to allocate from
  * @src: buffer to read the packet from
  * @srclen: length of the @src (must be TOTAL_PACKET_SIZE)
- * @why_bad: if NULL return, this is what was wrong with the packet.
+ * @dest: the destination into which we should parse the packet
  */
-struct onionpacket *parse_onionpacket(const tal_t *ctx,
+enum onion_type parse_onionpacket(const u8 *src,
-				      const void *src,
 				  const size_t srclen,
-				      enum onion_type *why_bad);
+				  struct onionpacket *dest);
 
 struct onionreply {
 	/* Node index in the path that is replying */

devtools/onion.c

@@ -103,21 +103,21 @@ static struct route_step *decode_with_privkey(const tal_t *ctx, const u8 *onion,
 {
 	struct privkey seckey;
 	struct route_step *step;
-	struct onionpacket *packet;
+	struct onionpacket packet;
 	enum onion_type why_bad;
 	u8 shared_secret[32];
 	if (!hex_decode(hexprivkey, strlen(hexprivkey), &seckey, sizeof(seckey)))
 		errx(1, "Invalid private key hex '%s'", hexprivkey);
 
-	packet = parse_onionpacket(ctx, onion, TOTAL_PACKET_SIZE, &why_bad);
+	why_bad = parse_onionpacket(onion, TOTAL_PACKET_SIZE, &packet);
 
-	if (!packet)
+	if (why_bad != 0)
 		errx(1, "Error parsing message: %s", onion_type_name(why_bad));
 
-	if (!onion_shared_secret(shared_secret, packet, &seckey))
+	if (!onion_shared_secret(shared_secret, &packet, &seckey))
 		errx(1, "Error creating shared secret.");
 
-	step = process_onionpacket(ctx, packet, shared_secret, assocdata,
+	step = process_onionpacket(ctx, &packet, shared_secret, assocdata,
 				   tal_bytelen(assocdata));
 	return step;
 

lightningd/pay.c

@@ -969,7 +969,7 @@ static struct command_result *json_sendonion(struct command *cmd,
 					     const jsmntok_t *params)
 {
 	u8 *onion;
-	struct onionpacket *packet;
+	struct onionpacket packet;
 	enum onion_type failcode;
 	struct htlc_out *hout;
 	struct route_hop *first_hop;
@@ -989,9 +989,9 @@ static struct command_result *json_sendonion(struct command *cmd,
 		   NULL))
 		return command_param_failed();
 
-	packet = parse_onionpacket(cmd, onion, tal_bytelen(onion), &failcode);
+	failcode = parse_onionpacket(onion, tal_bytelen(onion), &packet);
 
-	if (!packet)
+	if (failcode != 0)
 		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 				    "Could not parse the onion. Parsing failed "
 				    "with failcode=%d",
@@ -1032,7 +1032,7 @@ static struct command_result *json_sendonion(struct command *cmd,
 		wallet_local_htlc_out_delete(ld->wallet, channel, payment_hash);
 	}
 
-	failcode = send_onion(cmd->ld, packet, first_hop, payment_hash, channel,
+	failcode = send_onion(cmd->ld, &packet, first_hop, payment_hash, channel,
 			  &hout);
 
 	payment = tal(hout, struct wallet_payment);

lightningd/peer_htlcs.c

@@ -894,7 +894,7 @@ static bool peer_accepted_htlc(struct channel *channel, u64 id,
 {
 	struct htlc_in *hin;
 	struct route_step *rs;
-	struct onionpacket *op;
+	struct onionpacket op;
 	struct lightningd *ld = channel->peer->ld;
 	struct htlc_accepted_hook_payload *hook_payload;
 
@@ -947,10 +947,10 @@ static bool peer_accepted_htlc(struct channel *channel, u64 id,
 
 	/* channeld calls both parse_onionpacket and process_onionpacket,
 	 * so they should succeed.. */
-	op = parse_onionpacket(tmpctx, hin->onion_routing_packet,
+	*failcode = parse_onionpacket(hin->onion_routing_packet,
 				      sizeof(hin->onion_routing_packet),
-			       failcode);
+				      &op);
-	if (!op) {
+	if (*failcode != 0) {
 		channel_internal_error(channel,
 				       "bad onion in got_revoke: %s",
 				       tal_hexstr(channel, hin->onion_routing_packet,
@@ -958,7 +958,7 @@ static bool peer_accepted_htlc(struct channel *channel, u64 id,
 		return false;
 	}
 
-	rs = process_onionpacket(tmpctx, op, hin->shared_secret->data,
+	rs = process_onionpacket(tmpctx, &op, hin->shared_secret->data,
 				 hin->payment_hash.u.u8,
 				 sizeof(hin->payment_hash));
 	if (!rs) {

wallet/test/run-wallet.c

@@ -477,10 +477,9 @@ struct command_result *param_tok(struct command *cmd UNNEEDED, const char *name
 				 const jsmntok_t **out UNNEEDED)
 { fprintf(stderr, "param_tok called!\n"); abort(); }
 /* Generated stub for parse_onionpacket */
-struct onionpacket *parse_onionpacket(const tal_t *ctx UNNEEDED,
+enum onion_type parse_onionpacket(const u8 *src UNNEEDED,
-				      const void *src UNNEEDED,
 				  const size_t srclen UNNEEDED,
-				      enum onion_type *why_bad UNNEEDED)
+				  struct onionpacket *dest UNNEEDED)
 { fprintf(stderr, "parse_onionpacket called!\n"); abort(); }
 /* Generated stub for payment_failed */
 void payment_failed(struct lightningd *ld UNNEEDED, const struct htlc_out *hout UNNEEDED,