zsh-workers
 help / color / mirror / code / Atom feed
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Zsh workers <zsh-workers@zsh.org>
Subject: Re: [PATCH] ztrftime(): Fix truncation for %.
Date: Sat, 29 Dec 2018 09:55:16 +0000	[thread overview]
Message-ID: <20181229095516.npdfdskue6yhnrtr@tarpaulin.shahaf.local2> (raw)
In-Reply-To: <EEC2E28C-7B10-4848-9C54-3F2B6261FBF7@dana.is>

dana wrote on Fri, Dec 28, 2018 at 16:16:03 -0600:
> On 24 Dec 2018, at 11:31, dana <dana@dana.is> 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

  reply	other threads:[~2018-12-29  9:55 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <d7b0451f90bdfe61f48cc1361690180e07158900.camel@ntlworld.com>
     [not found] ` <b8851c3a50bd8bceba1961f2f764e1a6869481ac.camel@ntlworld.com>
2018-12-20 22:25   ` The big kre zsh bug report Martijn Dekker
2018-12-21  7:53     ` Bart Schaefer
2018-12-21  8:11       ` Fix for ${\var} oddity Bart Schaefer
2018-12-25 17:18       ` [PATCH] honour NO_UNSET when reading values in arithmetic expansion/commands Martijn Dekker
2018-12-25 20:44       ` 'wait' exit status and warnings [was: The big kre zsh bug report] Martijn Dekker
2018-12-30 18:13         ` Peter Stephenson
2019-01-21 22:53           ` Martijn Dekker
2018-12-31  2:08       ` Line continuation between $ and { " Martijn Dekker
2018-12-21 11:30     ` The big kre zsh bug report Robert Elz
2018-12-21 20:37       ` Bart Schaefer
2018-12-22  0:13       ` Robert Elz
2018-12-21  2:28   ` Robert Elz
2018-12-24  5:40 ` zsh 5.6.2-test-2 Axel Beckert
2018-12-24  7:14   ` Axel Beckert
2018-12-24  7:38     ` dana
2018-12-24  9:16       ` [PATCH] ztrftime(): Fix truncation for % dana
2018-12-24 12:45         ` Daniel Shahaf
2018-12-24 16:24           ` dana
2018-12-24 17:06             ` Daniel Shahaf
2018-12-24 17:31               ` dana
2018-12-28 22:16                 ` dana
2018-12-29  9:55                   ` Daniel Shahaf [this message]
2018-12-29 10:27                     ` Daniel Shahaf
2018-12-29 11:02                       ` dana
2018-12-29 11:08                         ` Daniel Shahaf
2018-12-29 11:30                           ` dana
2018-12-29 11:34                             ` Daniel Shahaf
2018-12-24 23:35           ` Joey Pabalinas
2018-12-24 23:30         ` Joey Pabalinas
     [not found] ` <CAKc7PVDUjo8HAdwqgRAKcgQHOzThM+hYnjX+2FKzUZB+pfmC-Q@mail.gmail.com>
     [not found]   ` <CAKc7PVB-agFUarJ=LqC2QNDFta1O5D_o4v-gt7LiobVDohNGVQ@mail.gmail.com>
     [not found]     ` <06228a6975b91f7066d0046bf912dd69fa5993a2.camel@ntlworld.com>
2018-12-31 13:44       ` zsh 4.6.2-test-2 dana
2018-12-31 15:19         ` Sebastian Gniazdowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181229095516.npdfdskue6yhnrtr@tarpaulin.shahaf.local2 \
    --to=d.s@daniel.shahaf.name \
    --cc=zsh-workers@zsh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).