Re: timezone related bug of git

Jeff King [off-list ref] writes:

I won't be at all surprised if it has funny corner cases. Our
tm_to_time_t() is pretty basic and hacky. We can't use mktime() because
it only handles the current system timezone. OTOH, I think the tz_offset
we're undoing here originally came from comparing mktime() versus
tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out
any bugs exactly. :)

So maybe the code below is sufficient, but we'd probably at least want
some tests on top. Maybe something somebody interested would like to
pick up and run with?

It would be very hard to write a code that does not work correctly
on a timestamp created in the same zone in the same season. It is
easy to get the direction of the offset wrong and not notice with
such a test, but with another test to show a timestamp from a zone
in a different zone (or across season boundary in an area where
daylight saving time is s thing), such an error can easily be
caught.

In show_date(), we start from UNIX time and go to "struct tm" using
either the system gmtime_r() (after adjusting the value with the tz
offset of the original timestamp) or localtime_r() (when we are
trying to show the value in our local timestamp), but this codepath
needs to undo that.  Our tm_to_time_t() indeed is basic but should
work correctly on a broken down UTC.  So the caller needs to further
compensate for the tz offset.

I have to wonder why gm_time_t() needs to use two separate codepaths
for positive and negative tz_offset, while the new code here can get
away without.  Does it have something to do with the direction of
truncation during division and modulo operation?

Thanks.

On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:

I have to wonder why gm_time_t() needs to use two separate codepaths
for positive and negative tz_offset, while the new code here can get
away without.  Does it have something to do with the direction of
truncation during division and modulo operation?

Hmm. Unless I am missing something, this part of gm_time_t() is simply
over-complicating things:

  minutes = tz < 0 ? -tz : tz;
  minutes = (minutes / 100)*60 + (minutes % 100);
  minutes = tz < 0 ? -minutes : minutes;

We switch to doing the computation in absolute-value units, but then
restore the sign. But just:

  minutes = (tz / 100) * 60 + (tz % 100);

is equivalent and shorter. If tz is negative, then both terms will be
negative, which is what you want (they sum to a larger absolute-value
negative number). This comes from f80cd783c6 (date.c: add "show_date()"
function., 2005-05-06), so I don't see any sign that there was specific
thought given to some obscure handling. And indeed later fixes like
fbab835c03 ([PATCH] fix show_date() for positive timezones, 2005-05-18)
imply to me that the original was just confused.

Later we do:

  if (minutes > 0) {
          if (unsigned_add_overflows(time, minutes * 60))
                  die("Timestamp+tz too large: %"PRItime" +%04d",
                      time, tz);
  } else if (time < -minutes * 60)
          die("Timestamp before Unix epoch: %"PRItime" %04d", time, tz);

And that does need separate paths for the overflow check, since we're
checking different boundaries. I suspect for the strftime() code that we
wouldn't need similar checks, because the earlier ones would have caught
any problems (i.e., we would not get as far as having a "struct tm" that
represented something outside the range of our time_t).

-Peff

On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:

quoted

diff --git a/strbuf.c b/strbuf.c
index b22e981655..8b8b1900bc 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			strbuf_addstr(&munged_fmt, "%%");
 			fmt++;
 			break;
+		case 's':
+			strbuf_addf(&munged_fmt, "%"PRItime,
+				    tm_to_time_t(tm) -
+				    3600 * (tz_offset / 100) -
+				    60 * (tz_offset % 100));
+			fmt++;
+			break;

In show_date(), we start from UNIX time and go to "struct tm" using
either the system gmtime_r() (after adjusting the value with the tz
offset of the original timestamp) or localtime_r() (when we are
trying to show the value in our local timestamp), but this codepath
needs to undo that.  Our tm_to_time_t() indeed is basic but should
work correctly on a broken down UTC.  So the caller needs to further
compensate for the tz offset.

This made me wonder how things interact with format-local. There's some
subtlety, but I think all is well.

I hadn't planned to work further on this, but now having thought about
it some more, it seemed worth capturing that and putting the finishing
touches on a real commit.

-- >8 --
Subject: [PATCH] strbuf_addftime(): handle "%s" manually

The strftime() function has a non-standard "%s" extension, which prints
the number of seconds since the epoch. But the "struct tm" we get has
already been adjusted for a particular time zone; going back to an epoch
time requires knowing that zone offset. Since strftime() doesn't take
such an argument, round-tripping to a "struct tm" and back to the "%s"
format may produce the wrong value (off by tz_offset seconds).

Since we're already passing in the zone offset courtesy of c3fbf81a85
(strbuf: let strbuf_addftime handle %z and %Z itself, 2017-06-15), we
can use that same value to adjust our epoch seconds accordingly.

Note that the description above makes it sound like strftime()'s "%s" is
useless (and really, the issue is shared by mktime(), which is what
strftime() would use under the hood). But it gets the two cases for
which it's designed correct:

  - the result of gmtime() will have a zero offset, so no adjustment is
    necessary

  - the result of localtime() will be offset by the local zone offset,
    and mktime() and strftime() are defined to assume this offset when
    converting back (there's actually some magic here; some
    implementations record this in the "struct tm", but we can't
    portably access or manipulate it. But they somehow "know" whether a
    "struct tm" is from gmtime() or localtime()).

This latter point means that "format-local:%s" actually works correctly
already, because in that case we rely on the system routines due to
6eced3ec5e (date: use localtime() for "-local" time formats,
2017-06-15). Our problem comes when trying to show times in the author's
zone, as the system routines provide no mechanism for converting in
non-local zones. So in those cases we have a "struct tm" that came from
gmtime(), but has been manipulated according to our offset.

The tests cover the broken round-trip by formatting "%s" for a time in a
non-system timezone. We use the made-up "+1234" here, which has two
advantages. One, we know it won't ever be the real system zone (and so
we're actually testing a case that would break). And two, since it has a
minute component, we're testing the full decoding of the +HHMM zone into
a number of seconds. Likewise, we test the "-1234" variant to make sure
there aren't any sign mistakes.

There's one final test, which covers "format-local:%s". As noted, this
already passes, but it's important to check that we didn't regress this
case. In particular, the caller in show_date() is relying on localtime()
to have done the zone adjustment, independent of any tz_offset we
compute ourselves. These should match up, since our local_tzoffset() is
likewise built around localtime(). But it would be easy for a caller to
forget to pass in a correct tz_offset to strbuf_addftime(). Fortunately
show_date() does this correctly (it has to because of the existing
handling of %z), and the test continues to pass. So this one is just
future-proofing against a change in our assumptions.

Signed-off-by: Jeff King <redacted>
---
 cache.h         |  1 +
 date.c          |  2 +-
 strbuf.c        | 14 +++++++++++++-
 t/t0006-date.sh |  4 ++++
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index eba12487b9..aa6f380d10 100644
--- a/cache.h
+++ b/cache.h

@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);
 timestamp_t approxidate_relative(const char *date);
 void parse_date_format(const char *format, struct date_mode *mode);
 int date_overflows(timestamp_t date);
+time_t tm_to_time_t(const struct tm *tm);
 
 #define IDENT_STRICT	       1
 #define IDENT_NO_DATE	       2

diff --git a/date.c b/date.c
index c55ea47e96..84bb4451c1 100644
--- a/date.c
+++ b/date.c

@@ -9,7 +9,7 @@
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
  */
-static time_t tm_to_time_t(const struct tm *tm)
+time_t tm_to_time_t(const struct tm *tm)
 {
 	static const int mdays[] = {
 	    0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334

diff --git a/strbuf.c b/strbuf.c
index b22e981655..a569b99ab9 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -1006,7 +1006,12 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 
 	/*
 	 * There is no portable way to pass timezone information to
-	 * strftime, so we handle %z and %Z here.
+	 * strftime, so we handle %z and %Z here. Likewise '%s', because
+	 * going back to an epoch time requires knowing the zone.
+	 *
+	 * Note that tz_offset is in the "[-+]HHMM" decimal form; this is what
+	 * we want for %z, but the computation for %s has to convert to number
+	 * of seconds.
 	 */
 	for (;;) {
 		const char *percent = strchrnul(fmt, '%');

@@ -1019,6 +1024,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			strbuf_addstr(&munged_fmt, "%%");
 			fmt++;
 			break;
+		case 's':
+			strbuf_addf(&munged_fmt, "%"PRItime,
+				    tm_to_time_t(tm) -
+				    3600 * (tz_offset / 100) -
+				    60 * (tz_offset % 100));
+			fmt++;
+			break;
 		case 'z':
 			strbuf_addf(&munged_fmt, "%+05d", tz_offset);
 			fmt++;

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 6b757d7169..794186961e 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -63,6 +63,10 @@ check_show 'format-local:%%z' "$TIME" '%z'
 check_show 'format:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 16:13:20'
 check_show 'format-local:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 09:13:20' '' EST5
 
+check_show 'format:%s' '123456789 +1234' 123456789
+check_show 'format:%s' '123456789 -1234' 123456789
+check_show 'format-local:%s' '123456789 -1234' 123456789
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT

-- 
2.34.0.rc0.612.g98bfd90890

On Tue, Nov 02, 2021 at 07:35:35AM -0400, Jeff King wrote:

Looks like we may need something like this squashed in:

diff --git a/strbuf.c b/strbuf.c
index 33015b33df..995394f38e 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -1026,7 +1026,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			break;
 		case 's':
 			strbuf_addf(&munged_fmt, "%"PRItime,
-				    tm_to_time_t(tm) -
+				    (timestamp_t)tm_to_time_t(tm) -
 				    3600 * (tz_offset / 100) -
 				    60 * (tz_offset % 100));
 			fmt++;

because tm_to_time_t() returns an actual time_t, which will vary in
size. The 32-bit CI job complains:

  strbuf.c:1028:29: error: format '%lld' expects argument of type 'long long int', but argument 3 has type 'long int' [-Werror=format=]

-Peff