[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180802233121.GP1392@brightrain.aerifal.cx>
Date: Thu, 2 Aug 2018 19:31:21 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] strftime: fix %z sign for small negative time
zone offsets
On Thu, Aug 02, 2018 at 07:29:23PM -0400, Rich Felker wrote:
> On Wed, Jul 25, 2018 at 12:11:42PM +0200, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@...ecki.pl>
> >
> > Using + printf flag for printing plus/minus sign isn't reliable for
> > negative offsets greater than -3600. In such cases the first two digits
> > are zero but offset still should be printed with a leading minus char.
> >
> > Existing implementation results in code:
> > struct tm tm = { .tm_gmtoff = -1800, };
> > char buf[255];
> > strftime(buf, sizeof(buf), "%z", &tm);
> > puts(buf);
> > printing +0030 instead of -0030.
> >
> > This patch fixes it by handling sign character manually and checking the
> > __tm_gmtoff value (instead of __tm_gmtoff / 3600).
> >
> > Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
> > ---
> > src/time/strftime.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/time/strftime.c b/src/time/strftime.c
> > index 0a256970..6716ad4b 100644
> > --- a/src/time/strftime.c
> > +++ b/src/time/strftime.c
> > @@ -181,7 +181,8 @@ const char *__strftime_fmt_1(char (*s)[100], size_t *l, int f, const struct tm *
> > *l = 0;
> > return "";
> > }
> > - *l = snprintf(*s, sizeof *s, "%+.2ld%.2d",
> > + *l = snprintf(*s, sizeof *s, "%c%.2ld%.2d",
> > + tm->__tm_gmtoff >= 0 ? '+' : '-',
> > (tm->__tm_gmtoff)/3600,
> > abs(tm->__tm_gmtoff%3600)/60);
> > return *s;
> > --
>
> I think this is incorrect. tm->__tm_gmtoff/3600 can still be negative,
> and then the - sign is printed twice.
>
> I suspect your patch is correct if we add abs() around that expression
> too, but maybe it makes more sense to assemble the value to be printed
> as a single integer:
>
> tm->__tm_gmtoff/3600*100 + tm->__tm_gmtoff%3600/60
>
> I didn't spend a lot of time working out that this is correct, but it
> looks right and checks out with a few test cases.
Also, since testing has been a topic lately, I want to point out that
the cases that would have been broken by this patch, as well as the
ones it's intended to fix, should be test cases for strftime.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
