Re: [PATCH] zsh/random module [UPDATED]

[Clinton Bunch <cdb_zsh@zentaur.org>]

This is a from memory reply as I'm away from my git repository/code for 
the holiday. :)  So there may be a follow up.

On 11/23/2022 1:46 PM, Daniel Shahaf wrote:
> Clinton Bunch wrote on Mon, Nov 07, 2022 at 18:18:15 -0600:
> +++ b/Doc/Zsh/mod_random.yo
>> @@ -0,0 +1,67 @@
>> +startitem()
>> +findex(getrandom)
>> +cindex(random data, printing, array)
>> +xitem(tt(getrandom) [ tt(-c) var(count) ] [ tt(-r) ] [ tt(-s) var(scalar) ] [tt(-i) ] [ tt(-a) var(array) ] [ tt(-L) var(lower_bound) ] [ tt(-U) var(upper_bound) ])
> Never use xitem() without item() after it.
I'm using existing docs as a template as I don't know YODL at all, so 
I'm not surprised I made mistakes.
>
> I don't understand this API.
>
> - Magic numbers:
>
>    + Why should -c default to 8 in the "random bytes" case?  Why
>      shouldn't it default to some other value, or even be required to be
>      specified explicitly by the user?
Not sure why 8 was the number I thought of, just that one wasn't 
particularly useful.
>
>    + Why should -c0 be an error?  It should be valid to request an array
>      of 0 random numbers, just like, say, «yes | head -n 0».
I thought nonsensical options must be an error.
>
>    + Why should -c65 be an error?  (Saw your comment elsethread that you
>      imagine that would suffice.  That's not a good reason for this limit.)
It started as a 256 byte limit from getrandom().
>
>    + Why should -L$(( 2**32 - 1)) be an error?  I should be able to request
>      a random integer in the range { x | 42 ≤ x ≤ 42 }.  It's perfectly
>      well-defined.
if the difference between lower and upper is zero, you get a divide by 
zero error unless you special case it, and it never occurred to me that 
someone would specify that case deliberately
>
> - Why should -c's default depend on anything at all?  You mentioned
>    downthread you consider making -c1 the default in all cases; that'd be
>    better.
>
> - The builtin can generate either integers or bytes.  Most of the API
>    (the type of "returned random data" that -c deals with, the default
>    number of those, validity of -L/-U, etc.) is split right along this
>    line.  Shouldn't this become two separate builtins, then?  One for
>    bytes and one for integers?
>
>    (More on this below, at the end of the implementation review.)
>
> - Interdependencies.  This can generate either bytes or unsigned ints,
>    and bytes can be emitted either in hex, raw, or in decimal, and either
>    to stdout or to a scalar or to an array… but not all of these
>    combinations are possible.  For instance, adding «-a» to «getrandom -c 1»
>    changes the output encoding from hex to decimal AND the output channel
>    from stdout to $reply.  It seems better to avoid coupling things like
>    this.
>
>> +subsect(Parameters)
>> +
>> +startitem()
>> +vindex(SRANDOM)
>> +item(tt(SRANDOM)) (
>> +A random positive 32-bit integer between 0 and 4,294,967,295.  This parameter
>> +is read-only.
>> +)
>> +enditem()
>> +
>> +subsect(Math Functions)
>> +
>> +startitem()
>> +item(tt(zrandom+LPAR()RPAR())) (
> Doesn't this need a findex()?
>
> Do other math functions have index entries?  And if not (systell()
> doesn't), shouldn't they?
>
>> +++ b/Src/Modules/random.c
>> @@ -0,0 +1,675 @@
> There's a whole bunch of orthographic issues in this file (whitespace,
> punctuation, spelling, etc.).  I won't point them out now since there
> are bigger issues.
>
>> +#include <stdbool.h>
> C99-ism.  (zsh is written in C89.)
>
>> +/* buffer to pre-load integers for SRANDOM to lessen the context switches */
>> +uint32_t rand_buff[8];
> C99-ism (the array element type).

I asked about POSIX standards as the dev guide seemed badly out of date 
in workers/50819

Got this response in workers/50847


On Tue, Oct 25, 2022 at 6:52 PM Clinton Bunch <cdb_zsh@zentaur.org> wrote:
 >
 > My personal opinion is that development should use at least the
 > POSIX-1.2001 standard.  It's 20 years old.  That's surely old enough for
 > any system still running.  (It's  certainly old enough that any such
 > system is not supported by the vendor)

OTOH any system not supported by the vendor is exactly one where
somebody might be trying to build their own zsh binary.

That said, I thought we standardized on c99 rather than c89 quite some 
time ago.

>
> Even in C99, wouldn't this require an explicit #include?  I guess it
> works now due to transitive #include's, but I don't know that it's
> guaranteed to work everywhere.
>
>> +getrandom_buffer(void *buf, size_t len)
>> +{
> ⋮
>> +    do {
>> +#ifdef HAVE_ARC4RANDOM
>> +	/* Apparently, the creaters of arc4random didn't believe in errors */
> Leave out the /ad homines/.
>
> FWIW, I would sooner guess arc4random() was originally written on
> a platform where it /couldn't/ error.
>
>> +	arc4random_buf(buf,len);
>> +	ret = len;
>> +#elif defined(HAVE_GETRANDOM)
>> +	ret=getrandom(bufptr,(len - val),0);
>> +#else
>> +	ret=read(randfd,bufptr,(len - val));
> The premise of the module is to offer an interface to high-quality
> random numbers.  Is /dev/urandom, which is the module falls back to
> here, necessarily of sufficient quality?  If not, shouldn't there be
> a way for the module's user to determine the source of randomness?
arc4random and getrandom are just more efficient access to the pool of 
/dev/urandom.   AFAIK it's as good a pseudo-random source as any on any 
platform.
>
>> +#endif
>> +	if (ret < 0) {
>> +	    if (errno != EINTR || errflag || retflag || breaks || contflag) {
>> +		zwarn("Unable to get random data: %e.", errno);
> Need to set «errno = 0» first, surely?
>
>> +/*
>> + * Implements the getrandom builtin to return a string of random bytes of
>> + * user-specified length.  It either prints them to stdout or returns them
>> + * in a parameter passed on the command line.
>> + *
>> + */
>> +
>> +/**/
>> +static int
>> +bin_getrandom(char *name, char **argv, Options ops, int func)
>> +{
> ⋮
>> +    bool integer_out = false;	 /*  boolean */
> The target audience for comments knows C.  So, nuke this comment.
>
> You might define inline enums here (for ints/bytes, scalar/array/stdout,
> and decimal/hex/raw) to make the code more self-documenting.
>
> Incidentally, this would also immediately raise the question of why some
> combinations of these aren't valid (e.g., hex or raw bytes output to an
> array).
>
>> +    int i, j;		         /* loop counters */
>> +
>> +    /*maximum string length of a 32-bit integer + null */
>> +    char int2str[11];
> Have the comment point out that this doesn't include a sign byte?
>
> FWIW, we have a 64-bit version of this in DIGBUFSIZE.
>
>> +    /* Vailues for -U *nd -L */
>> +    zulong   upper = UINT32_MAX, lower = 0;
>> +    uint32_t diff  = UINT32_MAX;
>> +    uint32_t min   = 0, rej = 0;  /* Reject below min and count */
> «rej» is never read.
>
>> +    bool     bound = false;       /* set if upper or lower bound specified */
> Why is this variable needed?  Having it at all means that passing «-L 0
> -U $((2**32-1))» is different to not passing any -L or -U option at all,
> even though these values are ostensibly the defaults.
>
>> +    if (OPT_ISSET(ops, 'U')) {
>> +	if (!zstrtoul_underscore(OPT_ARG(ops, 'U'), &upper)) {
>> +	    zwarnnam(name, "Couldn't convert -U %s to a number.",
>> +		    OPT_ARG(ops, 'U'));
>> +	    return 1;
>> +	} else if (upper > UINT32_MAX || upper < 1) {
>> +	    zwarnnam(name,
>> +		    "Upper bound must be between 1 and %z you specified: %z.",
>> +		     (zlong) UINT32_MAX, (zlong) upper);
> If «upper > ZLONG_MAX», the downcast to zlong would result in
> a negative number.
zwarnnam has no zulong format and upper is supposed to be a 32-bit integer.
>
>> +	    return 1;
> ⋮
>> +    if (bound) {
> ⋮
>> +	if (lower > upper) {
>> +	    zwarnnam(name, "-U must be larger than -L.");
>> +	    return 1;
>> +	}
>> +
>> +	diff = upper == UINT32_MAX && lower == 0 ? UINT32_MAX :
>> +		upper - lower + 1;
> How can this possibly be right?  As written it means the meaning of
> «diff» with the default values of «upper»/«lower» is different to its
> meaning in literally every other case.
>
> Ah, I see.  Without special-casing the default values, explicitly
> passing «-L 0 -U $((2**32 - 1))» would incorrectly trigger the
> zwarnnam() just below, because «diff» is a uint32_t.  Thus, the
> special-casing is an incorrect fix to a real issue.
>
>> +	if(!diff) {
>> +	    zwarnnam(name, "Upper and Lower bounds cannot be the same.");
>> +	    return 1;
>> +	}
> This warning will never be printed.  [Why?  Because «diff» is either
> UINT32_MAX, which is true, or «(uint32_t)(zulong)(upper - lower + 1)»,
> which, taking into account the range checks on «upper» and «lower», can
> only work out to 0 if «upper == UINT32_MAX && lower == 0»… which is
> precisely the case that's special-cased in the assignment to «diff» just
> above.]
>
> I suppose it was intended to be printed for «getrandom -L 42 -U 42».  In
> practice, that incantation just prints 42.  As I mentioned in the API
> review, I think that's desired behaviour.  In either case, please add
> a test.
>
>> +    }
>> +
>> +    if(!OPT_ISSET(ops,'c'))
>> +	if ((!bound && !integer_out) || arrname)
>> +	len = 8;
> So one default value of -c is here and the other is up in the
> declaration of «len».  That's somewhat confusing.
>
>> +
>> +
>> +    if (!byte_len)
>> +	byte_len = len;
> The condition is always true.
>
> Didn't you get a compiler warning for this?
No.  Not in Developer's Studio, clang, gcc, or HP ANSI C.
>
>> +
>> +
>> +    if (bound) {
>> +	pad = len / 16 + 1;
>> +	byte_len = (len + pad) * sizeof(uint32_t);
> «pad» should be declared in this scope since it's not used elsewhere.
> Ditto «outbuff» below and others.
>
> Why «len / 16 + 1»?
I couldn't find any statistical information that would tell me how many 
numbers were likely to be rejected, so I guessed.
>> +    } else if (integer_out)
>> +	byte_len = len * sizeof(uint32_t);
>> +
>> +    if (byte_len > 256)
>> +	byte_len=256;
>> +
> Why not report an error to the user?
>
> ⋮
>> +    min = -diff % diff;
> See above about the special casing in the assignment to «diff».  I guess
> it affects this line.
>
> ⋮
>> +	sprintf(int2str, "%u", int_val);
> Hold on.  %u expects an «unsigned int» and «int_val» is a «uint32_t».
> So, this line is only correct when «unsigned int» happens to be a 32-bit
> type… while there's nothing stopping that type from being a 64-bit type,
> or even a 16-bit type, and in either case this line would read the wrong
> number of bytes off the varargs.
>
>> +	if (arrname) {
>> +	    array[i]=ztrdup(int2str);
>> +	} else if (scalar && len == 1) {
>> +	    setiparam(scalar, int_val);
>> +	} else {
>> +	    printf("%s ",int2str);
>> +	}
>> +    }
>> +    if (arrname) {
>> +	setaparam(arrname,array);
>> +    } else if (!scalar) {
>> +	printf("\n");
>> +    }
>> +
>> +
>> +    zfree(buf, byte_len);
>> +    return 0;
>> +}
>> +
> In the API docs review I proposed to split «getrandom» into two separate
> builtins.
>
> Having read this function, I reiterate that proposal.  This function,
> with all the if/then cases that try to keep track of which of the
> function's different modes of operation was requested, is difficult to
> read and to modify.
>
>> +
>> +/*
>> + * Provides for the SRANDOM parameter and returns an unsigned 32-bit random
>> + * integer.
>> + */
>> +
>> +/**/
>> +static zlong
>> +get_srandom(UNUSED(Param pm)) {
>> +
>> +    if(buf_cnt < 0) {
>> +	getrandom_buffer((void*) rand_buff,sizeof(rand_buff));
>> +	buf_cnt=7;
> Magic number.  You might want «sizeof(rand_buff) / sizeof(rand_buff[0]) - 1»…
>
> …but it'd be more idiomatic to leave that «- 1» out and then change the
> postfix decrement to a prefix decrement.  That would also be more
> consistent with the variable's name.  (As written, «buf_cnt == 0» means
> there's one more zlong in it; it's not a "count" but a "largest valid
> index".)
>
>> +    }
>> +    return rand_buff[buf_cnt--];
>> +}
>> +
>> +/*
>> + * Implements the mathfunc zrandom and returns a random floating-point
>> + * number between 0 and 1.  Does this by getting a random 32-bt integer
>> + * and dividing it by UINT32_MAX.
>> + *
> Presumably this means to guarantee a /uniformly-distributed/ random
> floating-point number in the given range.  So, why does "getting
> a random 32-bit integer and dividing it by UINT32_MAX" actually
> implement that guarantee?
>
> Oh, wait, never mind.  The comment is out of date.  Fix it.
>
>> + */
>> +
>> +/**/
>> +static mnumber
>> +math_zrandom(UNUSED(char *name), UNUSED(int argc), UNUSED(mnumber *argv),
>> +	     UNUSED(int id))
>> +{
>> +    mnumber ret;
>> +    double r;
>> +
>> +    r = random_real();
>> +    if (r < 0) {
>> +	zwarnnam(name, "Failed to get sufficient random data.");
> Say why.  (Probably via errno?)
>
> Rule of thumb: every error message should include at least one replaceable.
>
> (Further instances of this elsewhere in the file.)
>
>> +/**/
>> +int
>> +setup_(UNUSED(Module m))
>> +{
>> +#ifdef USE_URANDOM
>> +    /* Check for the existence of /dev/urandom */
>> +
>> +    struct stat st;
>> +
>> +    if (lstat("/dev/urandom",&st) < 0) {
> Why not stat()?
Is it appropriate for /dev/urandom to be a symlink?
>
>> +	zwarn("No kernel random pool found.");
>> +	return 1;
>> +    }
>> +
>> +    if (!(S_ISCHR(st.st_mode)) ) {
>> +	zwarn("No kernel random pool found.");
>> +	return 1;
>> +    }
>> +#endif /* USE_URANDOM */
>> +    return 0;
>> +}
>> +/**/
>> +int
>> +_zclz64(uint64_t x) {
> ⋮
>> +    if (!(x & 0xFFFFFFFF00000000)) {
> Don't you need ZLONG_CONST() here?
>
>> +/**/
>> +uint64_t
>> +random64(void) {
>> +    uint64_t r;
>> +
>> +    if(getrandom_buffer(&r,sizeof(r)) < 0) {
>> +	zwarn("zsh/random: Can't get sufficient random data.");
>> +	return 1; /* 0 will cause loop */
> Where is the docstring of random64() itself?  /That/ is what determines
> whether or not returning 0 would be correct.
>
> What loops and why?
>
> Also, I suspect this name will collide with a third-party function one
> day.  Recommending to rename.
>
>> +    }
>> +
>> +    return r;
>> +}
>> +
>> +/* Following code is under the below copyright, despite changes for error
>> + * handling and removing GCCisms */
>> +
> -1.  Don't say "Following code"; identify the code explicitly, or better
> yet, move it to its own *.c file.  Otherwise, any function we might
> append to this file would be taken to be covered by this copyright
> notice and license statement.
>
>> +/*
>> + * random_real: Generate a stream of bits uniformly at random and
>> + * interpret it as the fractional part of the binary expansion of a
>> + * number in [0, 1], 0.00001010011111010100...; then round it.
>> + */
>> +
>> +/**/
>> +double
> Need «static».
>
> (Further instances of this elsewhere in the file.)
>
>> +random_real(void)
>> +{
> ⋮
>> +}
>> +++ b/Test/V15random.ztst
>> @@ -0,0 +1,51 @@
>> +# Tests for the zsh/random module
>> +%test
>> +  getrandom -U 56
>> +1f:Checking if system has a kernel random source
>> +?(eval):1: No kernel random pool found.
>> +
> The test point's code has nothing to do with the test point's
> description and expected errput.
>
> The 'f' flag is why this test point's failure doesn't get reported.
The test is supposed to fail.
>
>> +  getrandom -U 64
>> +0:Checking upper Bound is observed
>> +*><0-64>
>> +
>> +  tmpmsg="failed"
>> +  getrandom -L 4 -U 5 -c 16 -a tmpa
>> +  for i in ${tmpa[@]}
>> +  do
>> +    if (( i == 5 )); then
>> +      tmpmsg="success"
>> +      break
>> +    fi
>> +  done
> Use ${tmpa[(I)5]} ?
>
>> +  echo $tmpmsg
>> +0:Checking that upper bound is inclusive
>> +F:Try running the test again: make TESTNUM=V15 check
> Baby and bathwater.  Say what the problem is first and what the
> recommended fix is second.  In this case, something along the lines
> of "This test WILL false positive once every 65536 runs on average.  To
> rule this out, run the test again."
>
> Oh, and bump that 16 to something 3 or 4 times as big, because a 1/65536
> chance isn't really enough in a world where automated builds (CI,
> distros' QA, etc.) is a thing.
>
>> +>success
>> +
>> +  echo $(( zrandom() ))
>> +0:Checking if zrandom() produces a floating-point number between 0 and 1
>> +*>0(|.)[[:digit:]]#
> Why is the period optional?  Something like "042" shouldn't be accepted.
>
>> diff --git a/configure.ac b/configure.ac
>> index 074141d38..f1fa01274 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -675,6 +675,7 @@ fi
>>   AC_CHECK_HEADERS(sys/time.h sys/times.h sys/select.h termcap.h termio.h \
>>   		 termios.h sys/param.h sys/filio.h string.h memory.h \
>>   		 limits.h fcntl.h libc.h sys/utsname.h sys/resource.h \
>> +                 sys/random.h \
> Tabs/spaces are wrong.  Also in the next hunk.
>
>>   		 locale.h errno.h stdio.h stdarg.h varargs.h stdlib.h \
>>   		 unistd.h sys/capability.h \
>>   		 utmp.h utmpx.h sys/types.h pwd.h grp.h poll.h sys/mman.h \
>> @@ -1337,6 +1338,7 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
>>   	       cygwin_conv_path \
>>   	       nanosleep \
>>   	       srand_deterministic \
>> +               getrandom arc4random \
>>   	       setutxent getutxent endutxent getutent)
>>   AC_FUNC_STRCOLL
>>   
> Bottom line:
>
> - Interfacing to kernel random number APIs seems reasonable enough.
>
> - The API to shell scripts feels unintuitive.
>
> - As the patch stands, it is not ready to be merged (even assuming
>    arguendo the API is perfect as-is).
>
> Thanks,
>
> Daniel
>