[PATCH] strftime: fix %z sign for small negative time zone offsets

[PATCH] strftime: fix %z sign for small negative time zone offsets

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.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@milecki.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;
-- 
2.13.7

Re: [PATCH] strftime: fix %z sign for small negative time zone offsets

[Rich Felker]

On Wed, Jul 25, 2018 at 12:11:42PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.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@milecki.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.

Rich

Re: [PATCH] strftime: fix %z sign for small negative time zone offsets

[Rich Felker]

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@milecki.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@milecki.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

Re: [PATCH] strftime: fix %z sign for small negative time zone offsets

[Rich Felker]

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@milecki.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@milecki.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.

I've thrown various values at it and it seems to work fine, so I'll
commit this version.

Rich