Update ccan/structeq.

structeq() is too dangerous: if a structure has padding, it can fail
silently.

The new ccan/structeq instead provides a macro to define foo_eq(),
which does the right thing in case of padding (which none of our
structures currently have anyway).

Upgrade ccan, and use it everywhere.  Except run-peer-wire.c, which
is only testing code and can use raw memcmp(): valgrind will tell us
if padding exists.

Interestingly, we still declared short_channel_id_eq, even though
we didn't define it any more!

Signed-off-by: Rusty Russell <[email protected]>

bitcoin/block.h

@@ -4,12 +4,15 @@
 #include "bitcoin/shadouble.h"
 #include <ccan/endian/endian.h>
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 #include <ccan/tal/tal.h>
 #include <stdbool.h>
 
 struct bitcoin_blkid {
 	struct sha256_double shad;
 };
+/* Define bitcoin_blkid_eq (no padding) */
+STRUCTEQ_DEF(bitcoin_blkid, 0, shad.sha.u);
 
 struct bitcoin_block_hdr {
 	le32 version;

bitcoin/preimage.h

@@ -2,8 +2,12 @@
 #define LIGHTNING_BITCOIN_PREIMAGE_H
 #include "config.h"
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 
 struct preimage {
 	u8 r[32];
 };
+/* Define preimage_eq */
+STRUCTEQ_DEF(preimage, 0, r);
+
 #endif /* LIGHTNING_BITCOIN_PREIMAGE_H */

bitcoin/privkey.h

@@ -2,11 +2,14 @@
 #define LIGHTNING_BITCOIN_PRIVKEY_H
 #include "config.h"
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 
 /* General 256-bit secret, which must be private.  Used in various places. */
 struct secret {
 	u8 data[32];
 };
+/* Define secret_eq */
+STRUCTEQ_DEF(secret, 0, data);
 
 /* This is a private key.  Keep it secret. */
 struct privkey {

bitcoin/pubkey.c

@@ -3,7 +3,6 @@
 #include <assert.h>
 #include <ccan/mem/mem.h>
 #include <ccan/str/hex/hex.h>
-#include <ccan/structeq/structeq.h>
 #include <common/type_to_string.h>
 #include <common/utils.h>
 
@@ -61,6 +60,7 @@ char *pubkey_to_hexstr(const tal_t *ctx, const struct pubkey *key)
 	pubkey_to_der(der, key);
 	return tal_hexstr(ctx, der, sizeof(der));
 }
+REGISTER_TYPE_TO_STRING(pubkey, pubkey_to_hexstr);
 
 char *secp256k1_pubkey_to_hexstr(const tal_t *ctx, const secp256k1_pubkey *key)
 {
@@ -74,13 +74,6 @@ char *secp256k1_pubkey_to_hexstr(const tal_t *ctx, const secp256k1_pubkey *key)
 }
 REGISTER_TYPE_TO_STRING(secp256k1_pubkey, secp256k1_pubkey_to_hexstr);
 
-bool pubkey_eq(const struct pubkey *a, const struct pubkey *b)
-{
-	return structeq(&a->pubkey, &b->pubkey);
-}
-
-REGISTER_TYPE_TO_STRING(pubkey, pubkey_to_hexstr);
-
 int pubkey_cmp(const struct pubkey *a, const struct pubkey *b)
 {
 	u8 keya[33], keyb[33];

bitcoin/pubkey.h

@@ -4,6 +4,7 @@
 #include <ccan/crypto/ripemd160/ripemd160.h>
 #include <ccan/crypto/sha256/sha256.h>
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 #include <ccan/tal/tal.h>
 #include <secp256k1.h>
 
@@ -15,6 +16,8 @@ struct pubkey {
 	/* Unpacked pubkey (as used by libsecp256k1 internally) */
 	secp256k1_pubkey pubkey;
 };
+/* Define pubkey_eq (no padding) */
+STRUCTEQ_DEF(pubkey, 0, pubkey.data);
 
 /* Convert from hex string of DER (scriptPubKey from validateaddress) */
 bool pubkey_from_hexstr(const char *derstr, size_t derlen, struct pubkey *key);
@@ -35,9 +38,6 @@ bool pubkey_from_der(const u8 *der, size_t len, struct pubkey *key);
 /* Pubkey to DER encoding: must be valid pubkey. */
 void pubkey_to_der(u8 der[PUBKEY_DER_LEN], const struct pubkey *key);
 
-/* Are these keys equal? */
-bool pubkey_eq(const struct pubkey *a, const struct pubkey *b);
-
 /* Compare the keys `a` and `b`. Return <0 if `a`<`b`, 0 if equal and >0 otherwise */
 int pubkey_cmp(const struct pubkey *a, const struct pubkey *b);
 

bitcoin/short_channel_id.h

@@ -2,18 +2,18 @@
 #define LIGHTNING_BITCOIN_SHORT_CHANNEL_ID_H
 #include "config.h"
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 #include <ccan/tal/tal.h>
 #include <stdbool.h>
 #include <stddef.h>
 
 /* Short Channel ID is composed of 3 bytes for the block height, 3
- * bytes of tx index in block and 2 bytes of output index. The
- * bitfield is mainly for unit tests where it is nice to be able to
- * just memset them and not have to take care about the extra byte for
- * u32 */
+ * bytes of tx index in block and 2 bytes of output index. */
 struct short_channel_id {
 	u64 u64;
 };
+/* Define short_channel_id_eq (no padding) */
+STRUCTEQ_DEF(short_channel_id, 0, u64);
 
 static inline u32 short_channel_id_blocknum(const struct short_channel_id *scid)
 {

bitcoin/tx.h

@@ -5,11 +5,14 @@
 #include "signature.h"
 #include "varint.h"
 #include <ccan/short_types/short_types.h>
+#include <ccan/structeq/structeq.h>
 #include <ccan/tal/tal.h>
 
 struct bitcoin_txid {
 	struct sha256_double shad;
 };
+/* Define bitcoin_txid_eq */
+STRUCTEQ_DEF(bitcoin_txid, 0, shad.sha.u);
 
 struct bitcoin_tx {
 	u32 version;

ccan/README

@@ -1,3 +1,3 @@
 CCAN imported from http://ccodearchive.net.
 
-CCAN version: init-2434-gac8694de
+CCAN version: init-2435-g92be2eff

ccan/ccan/crypto/shachain/shachain.h

@@ -116,6 +116,8 @@ bool shachain_add_hash(struct shachain *chain,
  *
  * Example:
  * #include <ccan/structeq/structeq.h>
+ * // Defines sha256_eq
+ * STRUCTEQ_DEF(sha256, 0, u);
  *
  * static void next_hash(const struct sha256 *hash)
  * {
@@ -127,7 +129,7 @@ bool shachain_add_hash(struct shachain *chain,
  *	else {
  *		struct sha256 check;
  *		assert(shachain_get_hash(&chain, index+1, &check));
- *		assert(structeq(&check, hash));
+ *		assert(sha256_eq(&check, hash));
  *	}
  * }
  */

ccan/ccan/structeq/LICENSE

@@ -1 +1 @@
-../../licenses/CC0
+../../licenses/BSD-MIT

ccan/ccan/structeq/_info

@@ -6,9 +6,11 @@
  * structeq - bitwise comparison of structs.
  *
  * This is a replacement for memcmp, which checks the argument types are the
- * same.
+ * same, and takes into account padding in the structure.  When there is no
+ * padding, it becomes a memcmp at compile time (assuming a
+ * constant-optimizing compiler).
  *
- * License: CC0 (Public domain)
+ * License: BSD-MIT
  * Author: Rusty Russell <[email protected]>
  *
  * Example:
@@ -19,26 +21,22 @@
  *	struct mydata {
  *		int start, end;
  *	};
+ *	// Defines mydata_eq(a, b)
+ *	STRUCTEQ_DEF(mydata, 0, start, end);
  *	
  *	int main(void)
  *	{
  *		struct mydata a, b;
  *	
- *		// No padding in struct, otherwise this doesn't work!
- *		BUILD_ASSERT(sizeof(a) == sizeof(a.start) + sizeof(a.end));
- *	
  *		a.start = 100;
  *		a.end = 101;
  *	
- *		b.start = 100;
- *		b.end = 101;
- *	
  *		// They are equal.
- *		assert(structeq(&a, &b));
+ *		assert(mydata_eq(&a, &b));
  *	
  *		b.end++;
  *		// Now they are not.
- *		assert(!structeq(&a, &b));
+ *		assert(!mydata_eq(&a, &b));
  *	
  *		return 0;
  *	}
@@ -50,6 +48,8 @@ int main(int argc, char *argv[])
 		return 1;
 
 	if (strcmp(argv[1], "depends") == 0) {
+		printf("ccan/build_assert\n");
+		printf("ccan/cppmagic\n");
 		return 0;
 	}
 

ccan/ccan/structeq/structeq.h

@@ -1,17 +1,45 @@
-/* CC0 (Public domain) - see LICENSE file for details */
+/* MIT (BSD) license - see LICENSE file for details */
 #ifndef CCAN_STRUCTEQ_H
 #define CCAN_STRUCTEQ_H
+#include <ccan/build_assert/build_assert.h>
+#include <ccan/cppmagic/cppmagic.h>
 #include <string.h>
+#include <stdbool.h>
 
 /**
- * structeq - are two structures bitwise equal (including padding!)
- * @a: a pointer to a structure
- * @b: a pointer to a structure of the same type.
+ * STRUCTEQ_DEF - define an ..._eq function to compare two structures.
+ * @sname: name of the structure, and function (<sname>_eq) to define.
+ * @padbytes: number of bytes of expected padding, or -1 if unknown.
+ * @...: name of every member of the structure.
  *
- * If you *know* a structure has no padding, you can memcmp them.  At
- * least this way, the compiler will issue a warning if the structs are
- * different types!
+ * This generates a single memcmp() call in the common case where the
+ * structure contains no padding.  Since it can't tell the difference between
+ * padding and a missing member, @padbytes can be used to assert that
+ * there isn't any, or how many we expect.  -1 means "expect some", since
+ * it can be platform dependent.
  */
-#define structeq(a, b) \
-	(memcmp((a), (b), sizeof(*(a)) + 0 * sizeof((a) == (b))) == 0)
+#define STRUCTEQ_DEF(sname, padbytes, ...)				\
+static inline bool CPPMAGIC_GLUE2(sname, _eq)(const struct sname *_a, \
+					      const struct sname *_b) \
+{									\
+	BUILD_ASSERT(((padbytes) < 0 &&					\
+		      CPPMAGIC_JOIN(+, CPPMAGIC_MAP(STRUCTEQ_MEMBER_SIZE_, \
+						    __VA_ARGS__))	\
+		      > sizeof(*_a))					\
+		     || CPPMAGIC_JOIN(+, CPPMAGIC_MAP(STRUCTEQ_MEMBER_SIZE_, \
+						      __VA_ARGS__))	\
+		     + (padbytes) == sizeof(*_a));			\
+	if (CPPMAGIC_JOIN(+, CPPMAGIC_MAP(STRUCTEQ_MEMBER_SIZE_, __VA_ARGS__)) \
+	    == sizeof(*_a))						\
+		return memcmp(_a, _b, sizeof(*_a)) == 0;		\
+	else								\
+		return CPPMAGIC_JOIN(&&,				\
+				     CPPMAGIC_MAP(STRUCTEQ_MEMBER_CMP_, \
+						  __VA_ARGS__));	\
+}
+
+/* Helpers */
+#define STRUCTEQ_MEMBER_SIZE_(m) sizeof((_a)->m)
+#define STRUCTEQ_MEMBER_CMP_(m) memcmp(&_a->m, &_b->m, sizeof(_a->m)) == 0
+
 #endif /* CCAN_STRUCTEQ_H */

ccan/ccan/structeq/test/compile_fail-different.c

@@ -8,6 +8,8 @@ struct mydata2 {
 	int start, end;
 };
 
+STRUCTEQ_DEF(mydata1, 0, start, end);
+
 int main(void)
 {
 	struct mydata1 a = { 0, 100 };
@@ -18,5 +20,5 @@ int main(void)
 #endif
 		b = { 0, 100 };
 
-	return structeq(&a, &b);
+	return mydata1_eq(&a, &b);
 }

ccan/ccan/structeq/test/run.c

@@ -5,6 +5,8 @@ struct mydata {
 	int start, end;
 };
 
+STRUCTEQ_DEF(mydata, 0, start, end);
+
 int main(void)
 {
 	struct mydata a, b;
@@ -14,13 +16,13 @@ int main(void)
 
 	a.start = 0;
 	a.end = 100;
-	ok1(structeq(&a, &a));
+	ok1(mydata_eq(&a, &a));
 
 	b = a;
-	ok1(structeq(&a, &b));
+	ok1(mydata_eq(&a, &b));
 
 	b.end++;
-	ok1(!structeq(&a, &b));
+	ok1(!mydata_eq(&a, &b));
 
 	/* This exits depending on whether all tests passed */
 	return exit_status();

channeld/channel.c

@@ -19,7 +19,6 @@
 #include <ccan/err/err.h>
 #include <ccan/fdpass/fdpass.h>
 #include <ccan/mem/mem.h>
-#include <ccan/structeq/structeq.h>
 #include <ccan/take/take.h>
 #include <ccan/tal/str/str.h>
 #include <ccan/time/time.h>
@@ -434,8 +433,8 @@ static void check_short_ids_match(struct peer *peer)
 	assert(peer->have_sigs[LOCAL]);
 	assert(peer->have_sigs[REMOTE]);
 
-	if (!structeq(&peer->short_channel_ids[LOCAL],
-		      &peer->short_channel_ids[REMOTE]))
+	if (!short_channel_id_eq(&peer->short_channel_ids[LOCAL],
+				 &peer->short_channel_ids[REMOTE]))
 		peer_failed(&peer->cs,
 			    &peer->channel_id,
 			    "We disagree on short_channel_ids:"
@@ -515,7 +514,7 @@ static void handle_peer_funding_locked(struct peer *peer, const u8 *msg)
 			    &peer->channel_id,
 			    "Bad funding_locked %s", tal_hex(msg, msg));
 
-	if (!structeq(&chanid, &peer->channel_id))
+	if (!channel_id_eq(&chanid, &peer->channel_id))
 		peer_failed(&peer->cs,
 			    &peer->channel_id,
 			    "Wrong channel id in %s (expected %s)",
@@ -547,7 +546,7 @@ static void handle_peer_announcement_signatures(struct peer *peer, const u8 *msg
 			    tal_hex(msg, msg));
 
 	/* Make sure we agree on the channel ids */
-	if (!structeq(&chanid, &peer->channel_id)) {
+	if (!channel_id_eq(&chanid, &peer->channel_id)) {
 		peer_failed(&peer->cs,
 			    &peer->channel_id,
 			    "Wrong channel_id: expected %s, got %s",

channeld/full_channel.c

@@ -4,7 +4,6 @@
 #include <bitcoin/tx.h>
 #include <ccan/array_size/array_size.h>
 #include <ccan/mem/mem.h>
-#include <ccan/structeq/structeq.h>
 #include <ccan/tal/str/str.h>
 #include <channeld/commit_tx.h>
 #include <channeld/full_channel.h>
@@ -334,7 +333,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 		if (old->state != htlc->state
 		    || old->msatoshi != htlc->msatoshi
 		    || old->expiry.locktime != htlc->expiry.locktime
-		    || !structeq(&old->rhash, &htlc->rhash))
+		    || !sha256_eq(&old->rhash, &htlc->rhash))
 			return CHANNEL_ERR_DUPLICATE_ID_DIFFERENT;
 		else
 			return CHANNEL_ERR_DUPLICATE;
@@ -515,7 +514,7 @@ enum channel_remove_err channel_fulfill_htlc(struct channel *channel,
 	 *  doesn't SHA256 hash to the corresponding HTLC `payment_hash`:
 	 *    - MUST fail the channel.
 	 */
-	if (!structeq(&hash, &htlc->rhash))
+	if (!sha256_eq(&hash, &htlc->rhash))
 		return CHANNEL_ERR_BAD_PREIMAGE;
 
 	htlc->r = tal_dup(htlc, struct preimage, preimage);

closingd/closing.c

@@ -1,6 +1,5 @@
 /* FIXME: We don't relay from gossipd at all here. */
 #include <bitcoin/script.h>
-#include <ccan/structeq/structeq.h>
 #include <closingd/gen_closing_wire.h>
 #include <common/close_tx.h>
 #include <common/crypto_sync.h>