On Mon, Aug 07, 2023 at 05:22:49PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
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
