shared: cleanup _get_hash_key_init() and better explain the reasoning

- add more code comments

- refactor the code flow in _get_hash_key_init() to follow a simpler
  code path.

- use c_siphash_hash() instead of 3 separate steps.

- Drop "?: static_seed" from nm_hash_static(). It's not useful, because
  the only _get_hash_key() for which _get_hash_key()^static_seed is zero
  is ~static_seed. That means, only one value of all the static seeds
  can result in zero here. At that point, we can just coerce that value
  to 3679500967u directly.

2 files changed, 49 insertions, 44 deletions

diff --git a/shared/nm-glib-aux/nm-hash-utils.c b/shared/nm-glib-aux/nm-hash-utils.c
index dcf662053a..0a701d06e1 100644
--- a/shared/nm-glib-aux/nm-hash-utils.c
+++ b/shared/nm-glib-aux/nm-hash-utils.c
@@ -24,7 +24,6 @@ static const guint8 *volatile global_seed = NULL;
 static const guint8 *
 _get_hash_key_init (void)
 {
-	static gsize g_lock;
 	/* the returned hash is aligned to guin64, hence, it is safe
 	 * to use it as guint* or guint64* pointer. */
 	static union {
@@ -34,50 +33,49 @@ _get_hash_key_init (void)
 		guint64 _align_as_uint64;
 	} g_arr;
 	const guint8 *g;
-	union {
-		guint8 v8[HASH_KEY_SIZE];
-		guint vuint;
-	} t_arr;
 
 again:
 	g = g_atomic_pointer_get (&global_seed);
-	if (G_LIKELY (g != NULL)) {
-		nm_assert (g == g_arr.v8);
-		return g;
-	}
-
-	{
-		CSipHash siph_state;
+	if (!G_UNLIKELY (g)) {
+		static gsize g_lock;
 		uint64_t h;
-
-		/* initialize a random key in t_arr. */
+		union {
+			guint vuint;
+			guint8 v8[HASH_KEY_SIZE];
+			guint8 _extra_entropy[3 * HASH_KEY_SIZE];
+		} t_arr;
 
 		nm_utils_random_bytes (&t_arr, sizeof (t_arr));
 
-		/* use siphash() of the key-size, to mangle the first guint. Otherwise,
-		 * the first guint has only the entropy that nm_utils_random_bytes()
-		 * generated for the first 4 bytes and relies on a good random generator.
+		/* We only initialize one random hash key. So we can spend some effort
+		 * of getting this right. For one, we collect more random bytes than
+		 * necessary.
 		 *
-		 * The first int is especially interesting for nm_hash_static() below, and we
-		 * want to have it all the entropy of t_arr. */
-		c_siphash_init (&siph_state, t_arr.v8);
-		c_siphash_append (&siph_state, (const guint8 *) &t_arr, sizeof (t_arr));
-		h = c_siphash_finalize (&siph_state);
-		if (sizeof (guint) < sizeof (h))
-			t_arr.vuint = t_arr.vuint ^ ((guint) (h & 0xFFFFFFFFu)) ^ ((guint) (h >> 32));
+		 * Then, the first guint of the seed should have all the entropy that we could
+		 * obtain in sizeof(t_arr). For that, siphash(t_arr) and xor the first guint
+		 * with hash.
+		 * The first guint is especially interesting for nm_hash_static() below that
+		 * doesn't use siphash itself. */
+		h = c_siphash_hash (t_arr.v8,
+		                    (const guint8 *) &t_arr,
+		                    sizeof (t_arr));
+		if (sizeof (h) > sizeof (guint))
+			t_arr.vuint = t_arr.vuint ^ ((guint) (h & G_MAXUINT)) ^ ((guint) (h >> 32));
 		else
-			t_arr.vuint = t_arr.vuint ^ ((guint) (h & 0xFFFFFFFFu));
-	}
+			t_arr.vuint = t_arr.vuint ^ ((guint) (h & G_MAXUINT));
+
+		if (!g_once_init_enter (&g_lock)) {
+			/* lost a race. The random key is already initialized. */
+			goto again;
+		}
 
-	if (!g_once_init_enter (&g_lock)) {
-		/* lost a race. The random key is already initialized. */
-		goto again;
+		memcpy (g_arr.v8, t_arr.v8, HASH_KEY_SIZE);
+		g = g_arr.v8;
+		g_atomic_pointer_set (&global_seed, g);
+		g_once_init_leave (&g_lock, 1);
 	}
 
-	memcpy (g_arr.v8, t_arr.v8, HASH_KEY_SIZE);
-	g = g_arr.v8;
-	g_atomic_pointer_set (&global_seed, g);
-	g_once_init_leave (&g_lock, 1);
+	nm_assert (g == g_arr.v8);
 	return g;
 }
 
@@ -94,18 +92,24 @@ again:
 guint
 nm_hash_static (guint static_seed)
 {
-	/* note that we only xor the static_seed with the key.
-	 * We don't use siphash, which would mix the bits better.
-	 * Note that this doesn't matter, because static_seed is not
-	 * supposed to be a value that you are hashing (for that, use
-	 * full siphash).
-	 * Instead, different callers may set a different static_seed
-	 * so that nm_hash_str(NULL) != nm_hash_ptr(NULL).
+	/* Note that we only xor the static_seed with the first guint of the key.
+	 *
+	 * We don't use siphash, which would mix the bits better with _get_hash_key().
+	 * Note that nm_hash_static() isn't used to hash the static_seed. Instead, it
+	 * is used to get a unique hash value in a static context. That means, every
+	 * caller is responsible to choose a static_seed that is sufficiently
+	 * distinct from all other callers. In other words, static_seed should be a
+	 * unique constant with good entropy.
+	 *
+	 * Note that _get_hash_key_init() already xored the first guint of the
+	 * key with the siphash of the entire static key. That means, even if
+	 * we got bad randomness for the first guint, the first guint is also
+	 * mixed with the randomness of the entire random key.
 	 *
-	 * Also, ensure that we don't return zero.
+	 * Also, ensure that we don't return zero (like for nm_hash_complete()).
 	 */
-	return ((*((const guint *) _get_hash_key ())) ^ static_seed)
-	       ?: static_seed ?: 3679500967u;
+	return    ((*((const guint *) _get_hash_key ())) ^ static_seed)
+	       ?: 3679500967u;
 }
 
 void
diff --git a/shared/nm-glib-aux/nm-hash-utils.h b/shared/nm-glib-aux/nm-hash-utils.h
index 9879dc477b..9f2e976678 100644
--- a/shared/nm-glib-aux/nm-hash-utils.h
+++ b/shared/nm-glib-aux/nm-hash-utils.h
@@ -88,7 +88,8 @@ nm_hash_complete (NMHashState *state)
 	/* we don't ever want to return a zero hash.
 	 *
 	 * NMPObject requires that in _idx_obj_part(), and it's just a good idea. */
-	return (((guint) (h >> 32)) ^ ((guint) h)) ?: 1396707757u;
+	return    (((guint) (h >> 32)) ^ ((guint) h))
+	       ?: 1396707757u;
 }
 
 static inline void