short_channel_id: make mk_short_channel_id return a failure.

We had a bug 0ba547ee10 caused by
short_channel_id overflow.  If we'd caught this, we'd have terminated
the peer instead of crashing, so add appropriate checks.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

bitcoin/short_channel_id.c

@@ -22,12 +22,17 @@
 */
 
 
-void mk_short_channel_id(struct short_channel_id *scid,
-			 u32 blocknum, u32 txnum, u16 outnum)
+bool mk_short_channel_id(struct short_channel_id *scid,
+			 u64 blocknum, u64 txnum, u64 outnum)
 {
+	if ((blocknum & 0xFFFFFF) != blocknum
+	    || (txnum & 0xFFFFFF) != txnum
+	    || (outnum & 0xFFFF) != outnum)
+		return false;
 	scid->u64 = (((u64)blocknum & 0xFFFFFF) << 40 |
 		     ((u64)txnum & 0xFFFFFF) << 16 |
 		     (outnum & 0xFFFF));
+	return true;
 }
 
 bool short_channel_id_from_str(const char *str, size_t strlen,
@@ -50,8 +55,8 @@ bool short_channel_id_from_str(const char *str, size_t strlen,
 #else
 	matches = sscanf(buf, "%ux%ux%hu", &blocknum, &txnum, &outnum);
 #endif
-	mk_short_channel_id(dst, blocknum, txnum, outnum);
-	return matches == 3;
+	return matches == 3
+		&& mk_short_channel_id(dst, blocknum, txnum, outnum);
 }
 
 char *short_channel_id_to_str(const tal_t *ctx, const struct short_channel_id *scid)

bitcoin/short_channel_id.h

@@ -47,8 +47,9 @@ static inline u16 short_channel_id_outnum(const struct short_channel_id *scid)
 	return scid->u64 & 0xFFFF;
 }
 
-void mk_short_channel_id(struct short_channel_id *scid,
-			 u32 blocknum, u32 txnum, u16 outnum);
+/* Returns false if blocknum, txnum or outnum require too many bits */
+bool WARN_UNUSED_RESULT mk_short_channel_id(struct short_channel_id *scid,
+					    u64 blocknum, u64 txnum, u64 outnum);
 
 bool WARN_UNUSED_RESULT short_channel_id_from_str(const char *str, size_t strlen,
 						  struct short_channel_id *dst);

gossipd/gossipd.c

@@ -664,13 +664,14 @@ static void reply_channel_range(struct peer *peer,
  * tail_blocks is the empty blocks at the end, in case they asked for all
  * blocks to 4 billion.
  */
-static void queue_channel_ranges(struct peer *peer,
+static bool queue_channel_ranges(struct peer *peer,
 				 u32 first_blocknum, u32 number_of_blocks,
 				 u32 tail_blocks)
 {
 	struct routing_state *rstate = peer->daemon->rstate;
 	u8 *encoded = encode_short_channel_ids_start(tmpctx);
 	struct short_channel_id scid;
+	bool scid_ok;
 
 	/* BOLT #7:
 	 *
@@ -688,10 +689,12 @@ static void queue_channel_ranges(struct peer *peer,
 
 	/* Avoid underflow: we don't use block 0 anyway */
 	if (first_blocknum == 0)
-		mk_short_channel_id(&scid, 1, 0, 0);
+		scid_ok = mk_short_channel_id(&scid, 1, 0, 0);
 	else
-		mk_short_channel_id(&scid, first_blocknum, 0, 0);
+		scid_ok = mk_short_channel_id(&scid, first_blocknum, 0, 0);
 	scid.u64--;
+	if (!scid_ok)
+		return false;
 
 	/* We keep a `uintmap` of `short_channel_id` to `struct chan *`.
 	 * Unlike a htable, it's efficient to iterate through, but it only
@@ -712,7 +715,7 @@ static void queue_channel_ranges(struct peer *peer,
 		reply_channel_range(peer, first_blocknum,
 				    number_of_blocks + tail_blocks,
 				    encoded);
-		return;
+		return true;
 	}
 
 	/* It wouldn't all fit: divide in half */
@@ -721,7 +724,7 @@ static void queue_channel_ranges(struct peer *peer,
 		/* We always assume we can send 1 blocks worth */
 		status_broken("Could not fit scids for single block %u",
 			      first_blocknum);
-		return;
+		return false;
 	}
 	status_debug("queue_channel_ranges full: splitting %u+%u and %u+%u(+%u)",
 		     first_blocknum,
@@ -729,10 +732,10 @@ static void queue_channel_ranges(struct peer *peer,
 		     first_blocknum + number_of_blocks / 2,
 		     number_of_blocks - number_of_blocks / 2,
 		     tail_blocks);
-	queue_channel_ranges(peer, first_blocknum, number_of_blocks / 2, 0);
-	queue_channel_ranges(peer, first_blocknum + number_of_blocks / 2,
-			     number_of_blocks - number_of_blocks / 2,
-			     tail_blocks);
+	return queue_channel_ranges(peer, first_blocknum, number_of_blocks / 2, 0)
+		&& queue_channel_ranges(peer, first_blocknum + number_of_blocks / 2,
+					number_of_blocks - number_of_blocks / 2,
+					tail_blocks);
 }
 
 /*~ The peer can ask for all channels is a series of blocks.  We reply with one
@@ -778,8 +781,12 @@ static u8 *handle_query_channel_range(struct peer *peer, const u8 *msg)
 	} else
 		tail_blocks = 0;
 
-	queue_channel_ranges(peer, first_blocknum, number_of_blocks,
-			     tail_blocks);
+	if (!queue_channel_ranges(peer, first_blocknum, number_of_blocks,
+				  tail_blocks))
+		return towire_errorfmt(peer, NULL,
+				       "Invalid query_channel_range %u+%u",
+				       first_blocknum, number_of_blocks + tail_blocks);
+
 	return NULL;
 }
 

lightningd/peer_control.c

@@ -537,9 +537,15 @@ static enum watch_result funding_lockin_cb(struct lightningd *ld,
 
 		loc = wallet_transaction_locate(tmpctx, ld->wallet, txid);
 		channel->scid = tal(channel, struct short_channel_id);
-		mk_short_channel_id(channel->scid,
-				    loc->blkheight, loc->index,
-				    channel->funding_outnum);
+		if (!mk_short_channel_id(channel->scid,
+					 loc->blkheight, loc->index,
+					 channel->funding_outnum)) {
+			channel_fail_permanent(channel, "Invalid funding scid %u:%u:%u",
+					       loc->blkheight, loc->index,
+					       channel->funding_outnum);
+			return DELETE_WATCH;
+		}
+
 		/* We've added scid, update */
 		wallet_channel_save(ld->wallet, channel);
 	}

wallet/wallet.c

@@ -2166,8 +2166,11 @@ wallet_outpoint_spend(struct wallet *w, const tal_t *ctx, const u32 blockheight,
 		assert(res == SQLITE_ROW);
 
 		scid = tal(ctx, struct short_channel_id);
-		mk_short_channel_id(scid, sqlite3_column_int(stmt, 0),
-				    sqlite3_column_int(stmt, 1), outnum);
+		if (!mk_short_channel_id(scid, sqlite3_column_int(stmt, 0),
+					 sqlite3_column_int(stmt, 1), outnum))
+			fatal("wallet_outpoint_spend: invalid scid %u:%u:%u",
+			      sqlite3_column_int(stmt, 0),
+			      sqlite3_column_int(stmt, 1), outnum);
 		db_stmt_done(stmt);
 		return scid;
 	}