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

[Junio C Hamano <gitster@pobox.com>]

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?
>
> 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"?

Thanks.

>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>> ---
>>  reftable/stack.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/reftable/stack.c b/reftable/stack.c
>> index 6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060652e72c26641749d 100644
>> --- a/reftable/stack.c
>> +++ b/reftable/stack.c
>> @@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
>>  		close(fd);
>>  		fd = -1;
>>  
>> -		delay = delay + (delay * rand()) / RAND_MAX + 1;
>> +		delay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) / UINT32_MAX + 1;
>>  		sleep_millisec(delay);
>>  	}
>>  
>> @@ -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(0);
>> +	uint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);
>>  	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
>>  		 min, max, rnd);
>>  	reftable_buf_reset(dest);