On Thu, Jul 29, 2021 at 07:35:24PM +0200, Theo Buehler wrote:
> On Thu, Jul 29, 2021 at 11:23:30AM -0600, Todd C. Miller wrote:
> > On Thu, 29 Jul 2021 07:11:26 -0600, "Theo de Raadt" wrote:
> >
> > > I'm not sure about the tradeoff in this approach.
> > >
> > > Won't the lack of precision in double eventually lead to non-uniformity?
> > >
> > > The way jot is written, I expect higher ranges to have lots of
> > > non-uniformity unless a substantial rewrite is undertaken, but I am
> > > worry your approach creates non-uniformity within the 32-bit range.
> >
> > I don't think it really makes a difference since the result of
> > "pow10 * (ender - begin)" will be a double regardless of whether
> > it is stored in a uint32_t or double.
>
> Also, doubles can precisely represent all uint32_t's, so I don't think
> it would have any effect on uniformity (uintx is only fed into
> arc4random_uniform()).
>
> > Changing the type of uintx to uint64_t will also fix the problem.
> > Perhaps that is more palatable?
>
> I think we should keep uintx as an uint32_t since that's what the
> arc4random() family expects. I'd argue that the check is wrong in that
> it should be done before assigning the double to an uint32_t.
>
> I'd suggest this diff:
>
> Index: jot.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/jot/jot.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 jot.c
> --- jot.c 27 Jun 2019 18:03:36 -0000 1.49
> +++ jot.c 29 Jul 2021 13:20:24 -0000
> @@ -244,7 +244,7 @@ main(int argc, char *argv[])
> if (putdata(x, reps == i && !infinity))
> errx(1, "range error in conversion: %f", x);
> } else { /* Random output: use defaults for omitted values. */
> - bool use_unif;
> + bool use_unif = 0;
> uint32_t pow10 = 1;
> uint32_t uintx = 0; /* Initialized to make gcc happy. */
>
> @@ -261,18 +261,19 @@ main(int argc, char *argv[])
> if (prec == 0 && (fmod(ender, 1) != 0 || fmod(begin, 1) != 0))
> use_unif = 0;
> else {
> + double range;
> +
> while (prec-- > 0)
> pow10 *= 10;
> - /*
> - * If pow10 * (ender - begin) is an integer, use
> - * arc4random_uniform().
> - */
> - use_unif = fmod(pow10 * (ender - begin), 1) == 0;
> - if (use_unif) {
> - uintx = pow10 * (ender - begin);
> - if (uintx >= UINT32_MAX)
> +
> + range = pow10 * (ender - begin);
> +
> + /* If range is an integer, use arc4random_uniform(). */
> + if (fmod(range, 1) == 0) {
> + if (range >= UINT32_MAX)
> errx(1, "requested range too large");
> - uintx++;
> + use_unif = 1;
> + uintx = range + 1;
> }
> }
>
>
That looks much better,
Alf
