Re: [PATCH 2/2] reftable/stack: accept insecure random bytes

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jan 07, 2025 at 01:03:26PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Yet a platform replaces it with a function that returns an error or
> > aborts?  What kind of nonsense is that?  Do we really need to cater
> > to such an insanity?
> >
> > Use of git_rand() here goes backwards against the more recent trend
> > in reftable/ directory to wean the code off of the rest of Git by
> > getting rid of unnecessary dependency, doesn't it?

It certainly does, yes, but unifying those two callsites to do the same
is also something I do in a patch series that gets rid of the last deps
on the Git codebase. This then allows me to move `git_rand()` into
"reftable/system.h" and make it a shim provided by the respective code
base that it's part of. So it's not a step backwards.

> > I think [PATCH 1/2] makes sense regardless, though.  But shouldn't
> > we be pushing back this step, with "fix your rand(3)"?
> >
> > Thanks.
> 
> Ah, I misread the patch.  It has two hunks, and what I said applies
> to the earlier one, but the later one is already contaminated with
> git_rand(), and that is what is failing, i.e. it is not a nonsense
> platform replacing rand() with something that can fail.
> 
> It may still make sense to drop the first hunk, and consider how to
> proceed when you further want to reduce the unnecessary dependencies
> for external users of the reftable library, though.  Are there
> correctness implications if git_rand() in format_name() yields non
> random results (like, always using "rnd = 0" instead of calling
> git_rand())?  I seriously hope not.  And if there is no correctness
> implications, perhaps we can replace it with rand() or even constant
> "0"?

No, there aren't any implications on correctness in that case. Sure, the
randomized delays not being randomized can lead to more contention. But
even when the randomized suffix for tables is deterministic we wouldn't
have an issue as the files are still distinguished by their update
indices.

Patrick