Re: [PATCH 1/6] maintenance: add get_random_minute()

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 8, 2023, 14:48 UTC
Message-ID: <ZNJVycLBUIXqZ0Xb@nand.local>
In-Reply-To: <xmqqzg329ycm.fsf@gitster.g>

On Mon, Aug 07, 2023 at 05:22:49PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> >> I was wondering where else we call srand() within Git, and it looks like
> >> the only other spot is in `lock_file_timeout()`.
> >
> > lock_file_timeout() should be updated to match git_mkstemps_mode(),
> > which was taught to use the csprng_bytes() function with 47efda96
> > (wrapper: use a CSPRNG to generate random file names, 2022-01-17),
> > and this new caller may want to do so as well, perhaps?  I dunno,
> > but the caller then does not have to worry about "initializing it
> > just once".
>
> Of course, the obvious downside is that crypto-secure one may be,
> unlike for its use in mkstemps(), way overkill for lockfiles and
> cron dispersion purposes, as these codepaths are not on the target
> surface.

I think that's an acceptable price to pay here, since we can drop the code to remember whether or not srand() has been called or not. Here's a patch that we could take in that direction:

--- 8< ---
Subject: [PATCH] lockfile.c: use a CSPRNG to generate backoff milliseconds

Upon failing to acquire the lockfile, `lock_file_timeout()` will try again with an exponential backoff. This backoff includes some noise as a multiplier over the default backoff behavior ranging from [0.75, 1.25].

It generates this noise via rand(3). Using a non-cryptographic source of randomness here is OK, since a more trivial attack vector (holding the file open via an external process for longer than the value of `timeout_ms`) is easier to exploit.

That all said, `lock_file_timeout()` initializes the PRNG with `srand()`. This has a couple of downsides:

  - lock_file_timeout() needs to remember whether or not the PRNG has
    or hasn't been seeded.

  - If a different function also calls `srand()`, the PRNG may start
    generating repeated values (if that caller also initialized the PRNG
    with `getpid()`).

Let's avoid both of these by using `csprng_bytes()`, in a similar spirit as 47efda967c (wrapper: use a CSPRNG to generate random file names, 2022-01-17).

Using a CSPRNG is definitely overkill for noising a backoff window, but it avoids the concerns about calling `srand()`, so let's use it here, too.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 lockfile.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 1d5ed01682..6587d407f4 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -107,22 +107,17 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 	int n = 1;
 	int multiplier = 1;
 	long remaining_ms = 0;
-	static int random_initialized = 0;

 	if (timeout_ms == 0)
 		return lock_file(lk, path, flags, mode);

-	if (!random_initialized) {
-		srand((unsigned int)getpid());
-		random_initialized = 1;
-	}
-
 	if (timeout_ms > 0)
 		remaining_ms = timeout_ms;

 	while (1) {
 		long backoff_ms, wait_ms;
 		int fd;
+		uint64_t rand;

 		fd = lock_file(lk, path, flags, mode);

@@ -135,7 +130,10 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,

 		backoff_ms = multiplier * INITIAL_BACKOFF_MS;
 		/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
-		wait_ms = (750 + rand() % 500) * backoff_ms / 1000;
+		if (csprng_bytes(&rand, sizeof(uint64_t)) < 0)
+			return error_errno(_("unable to get random bytes for"
+					     "lockfile backoff"));
+		wait_ms = (750 + rand % 500) * backoff_ms / 1000;
 		sleep_millisec(wait_ms);
 		remaining_ms -= wait_ms;

--
2.42.0.rc0.26.g802d811bac.dirty

--- >8 ---

Thanks,
Taylor