Subject: Re: [BUG] Git 2.48.0-rc2 - Test t0610.26 Fails

From: Patrick Steinhardt <[email-masked]>
Date: Tue, 7 Jan 2025 07:51:52 +0100

On Mon, Jan 06, 2025 at 08:42:51PM -0500, [email-masked] wrote:
> The following breaks at rc2. It worked correctly at rc1 on x86 but
> apparently not on ia64.
> 
> The OpenSSL random generator on ia64 uses PRNGD. On x86, the hardware
> randomizer
> is used. Nonetheless, this is not working properly:
[snip]
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes

This here is the underlying error. I cannot spot any single change in
v2.48.0-rc1..v2.48.0-rc2 that would cause a difference in how we compute
random bytes. Seeing that this is in the reftable library, the only
change that _could_ be related are the realloc followup-fixes, but that
could only be a sensible explanation in case we ever tried to populate a
reallocated buffer with N random bytes. We don't though, as there are
only two callsites where we compute random bytes:

  - `format_name()`

  - `reftable_stack_reload_maybe_reuse()`

In both cases we only want to have a single integer.

The latter callsite is somewhat curious because we don't call
`git_rand()`, but `rand()` directly. That wouldn't ever cause us to die,
so we know that it has to be `format_name()` that dies in `git_rand()`.

So yes, I doubt that this has been introduced with v2.48.0-rc2, and
rather assume that this is a flake that you just never hit before by
chance. Due to the many threads racing with one another, all of which
will repeatedly acquire random bytes, we probably end up exhausting the
pool of strong random entropy, and that makes OpenSSLs `RAND_bytes()`
fail.

It's somewhat sad that we die here, because it would be perfectly fine
to accept a pseudo-random number in our usecase. Using `rand()` does not
work work though because we never seed it, and I don't want to get into
that business, and thus it would return deterministic sequences of file
names for reftable stacks.

We _could_ adapt the code to use `RAND_pseudo_bytes()` for OpenSSL,
which never fails due to an exhausted entropy pool, but that'd first
require us to audit every callsite to figure out whether or not it needs
strong entropy. Another alternative would be to introduce a flag into
`csprng_bytes()` that tells it to also accept insecure bytes. That would
look something like the (not even compile-tested) diff at the end of
this mail.

Anyway. If my analysis is correct I don't think we need to rush a fix
into Git v2.48. The behaviour is preexisting, it's unlikely, and in the
error case we don't misbehave.

Patrick

diff --git a/builtin/gc.c b/builtin/gc.c
index a9b1c36de27..3e754f25bba 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1909,7 +1909,7 @@ static int get_random_minute(void)
 	if (getenv("GIT_TEST_MAINT_SCHEDULER"))
 		return 13;
 
-	return git_rand() % 60;
+	return git_rand(0) % 60;
 }
 
 static int is_launchctl_available(void)
diff --git a/reftable/stack.c b/reftable/stack.c
index 531660a49f0..8427c49026c 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st,
 static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
 {
 	char buf[100];
-	uint32_t rnd = (uint32_t)git_rand();
+	uint32_t rnd = (uint32_t)git_rand(CSPRNG_BYTES_INSECURE);
 	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
 		 min, max, rnd);
 	reftable_buf_reset(dest);
diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
index a4a0aca6177..c86dcc4870f 100644
--- a/t/helper/test-csprng.c
+++ b/t/helper/test-csprng.c
@@ -15,7 +15,7 @@ int cmd__csprng(int argc, const char **argv)
 
 	while (count) {
 		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
-		if (csprng_bytes(buf, chunk) < 0) {
+		if (csprng_bytes(buf, chunk, 0) < 0) {
 			perror("failed to read");
 			return 5;
 		}
diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 6b75a419b9d..f22b9775639 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -108,8 +108,8 @@ static void t_log_buffer_size(void)
 	   hash, to ensure that the compressed part is larger than the original.
 	*/
 	for (i = 0; i < REFTABLE_HASH_SIZE_SHA1; i++) {
-		log.value.update.old_hash[i] = (uint8_t)(git_rand() % 256);
-		log.value.update.new_hash[i] = (uint8_t)(git_rand() % 256);
+		log.value.update.old_hash[i] = (uint8_t)(git_rand(0) % 256);
+		log.value.update.new_hash[i] = (uint8_t)(git_rand(0) % 256);
 	}
 	reftable_writer_set_limits(w, update_index, update_index);
 	err = reftable_writer_add_log(w, &log);
@@ -325,7 +325,7 @@ static void t_log_zlib_corruption(void)
 	};
 
 	for (i = 0; i < sizeof(message) - 1; i++)
-		message[i] = (uint8_t)(git_rand() % 64 + ' ');
+		message[i] = (uint8_t)(git_rand(0) % 64 + ' ');
 
 	reftable_writer_set_limits(w, 1, 1);
 
diff --git a/wrapper.c b/wrapper.c
index fa79fd6ec9e..8b985931490 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -479,7 +479,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	for (count = 0; count < TMP_MAX; ++count) {
 		int i;
 		uint64_t v;
-		if (csprng_bytes(&v, sizeof(v)) < 0)
+		if (csprng_bytes(&v, sizeof(v), 0) < 0)
 			return error_errno("unable to get random bytes for temporary file");
 
 		/* Fill in the random bits. */
@@ -750,7 +750,7 @@ int open_nofollow(const char *path, int flags)
 #endif
 }
 
-int csprng_bytes(void *buf, size_t len)
+int csprng_bytes(void *buf, size_t len, MAYBE_UNUSED unsigned flags)
 {
 #if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)
 	/* This function never returns an error. */
@@ -785,14 +785,18 @@ int csprng_bytes(void *buf, size_t len)
 		return -1;
 	return 0;
 #elif defined(HAVE_OPENSSL_CSPRNG)
-	int res = RAND_bytes(buf, len);
-	if (res == 1)
+	switch (RAND_pseudo_bytes(buf, len)) {
+	case 1:
 		return 0;
-	if (res == -1)
-		errno = ENOTSUP;
-	else
+	case 0:
+		if (flags & CSPRNG_BYTES_INSECURE)
+			return 0;
 		errno = EIO;
-	return -1;
+		return -1;
+	default:
+		errno = ENOTSUP;
+		return -1;
+	}
 #else
 	ssize_t res;
 	char *p = buf;
@@ -816,11 +820,11 @@ int csprng_bytes(void *buf, size_t len)
 #endif
 }
 
-uint32_t git_rand(void)
+uint32_t git_rand(unsigned flags)
 {
 	uint32_t result;
 
-	if (csprng_bytes(&result, sizeof(result)) < 0)
+	if (csprng_bytes(&result, sizeof(result), flags) < 0)
 		die(_("unable to get random bytes"));
 
 	return result;
diff --git a/wrapper.h b/wrapper.h
index a6b3e1f09ec..7df824e34a9 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -127,18 +127,26 @@ int open_nofollow(const char *path, int flags);
 
 void sleep_millisec(int millisec);
 
+enum {
+	/*
+	 * Accept insecure bytes, which some CSPRNG implementations may return
+	 * in case the entropy pool has been exhausted.
+	 */
+	CSPRNG_BYTES_INSECURE = (1 << 0),
+};
+
 /*
  * Generate len bytes from the system cryptographically secure PRNG.
  * Returns 0 on success and -1 on error, setting errno.  The inability to
- * satisfy the full request is an error.
+ * satisfy the full request is an error. Accepts CSPRNG flags.
  */
-int csprng_bytes(void *buf, size_t len);
+int csprng_bytes(void *buf, size_t len, unsigned flags);
 
 /*
  * Returns a random uint32_t, uniformly distributed across all possible
- * values.
+ * values. Accepts CSPRNG flags.
  */
-uint32_t git_rand(void);
+uint32_t git_rand(unsigned flags);
 
 /* Provide log2 of the given `size_t`. */
 static inline unsigned log2u(uintmax_t sz)