Re: [PATCH] ztrftime(): Fix truncation for %.

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

dana wrote on Fri, Dec 28, 2018 at 16:16:03 -0600:
> On 24 Dec 2018, at 11:31, dana <dana@xxxxxxx> wrote:
> >Think i'll revisit it after Christmas :/
> 
> Maybe this is more reasonable...?

It's 0.100 correct.

(Sorry, I meant to say "99%", but I printed that line with unpatched
master ☺)

> +++ b/Src/utils.c
> @@ -3334,19 +3334,27 @@ morefmt:
>  #endif
>  	    switch (*fmt++) {
>  	    case '.':
> -		if (ztrftimebuf(&bufsize, digs))
> -		    return -1;
> +	    {
>  		if (digs > 9)
>  		    digs = 9;
> +		if (ztrftimebuf(&bufsize, digs))
> +		    return -1;
> +		long fnsec = nsec;
>  		if (digs < 9) {
> -		    int trunc;
> +		    int trunc, max = 10;
> +		    for (trunc = 1; trunc < digs; trunc++)
> +			max *= 10;
> +		    max -= 1;

As a matter of idiom, it would be clearer to initialize max to 1 (the
multiplicative identity) and trunc to 0 (so the loop body is run 'digs'
times).

Next, when «digs == 8», max will be multiplied by ten eight times (or
initialized to 10 and multiplied by ten seven times); however,
10⁸ > INT32_MAX, so when 'int' is a 32-bit type (as on x86 in Axel's
original report) max will overflow, wrap around, and not have the
intended value.

Using a 64-bit type (probably zlong) would address this — in that case
don't forget to adjust the sprintf() format string.

>  		    for (trunc = 8 - digs; trunc; trunc--)
> -			nsec /= 10;
> -		    nsec = (nsec + 8) / 10;
> +			fnsec /= 10;
> +		    fnsec = (fnsec + 5) / 10;
> +		    if (fnsec > max)
> +			fnsec = max;

With the int size change, this should be correct; however, since 'fnsec'
and 'max' are initialized separately, they could easily (after a future
code change) end up being out of sync by an order of magnitude.  That
is: it's correct, but perhaps not as robust as it could be.  As an
alternative, you could initialize 'max' to «1000000000ull» and divide it
by 10 in each iteration of the 'nsec' loop.  (There are other ways but
this one seems straightforward and doesn't run into wraparound edge cases.)

>  		}
> -		sprintf(buf, "%0*ld", digs, nsec);
> +		sprintf(buf, "%0*ld", digs, fnsec);
>  		buf += digs;
>  		break;
> +	    }

> +++ b/Test/V09datetime.ztst
> @@ -114,3 +114,17 @@
> +  strftime '%Y-%m-%d %H:%M:%S.%3.' 1012615322 $(( 999_999 ))
> +  for 1 in %. %1. %3. %6. %9. %12.; do
> +    print -rn - "$1 "
> +    strftime "%Y-%m-%d %H:%M:%S.$1" 1012615322 $(( 999_999_999 ))
> +  done
> +0:%. truncation

You mentioned upthread that this fixes a bug related to %. occurring
twice in the format string; add a test for that?

Bottom line, just change the int type and this'll be good to go. ☺

Cheers,

Daniel