Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime

Jeff King [off-list ref] writes:

This (non-)issue has consumed a lot more brain power than it is probably
worth. I'd like to figure out which patch to go with and be done. :)

Let's just deal with a simple known cases (like FreeBSD) in the real
code that everybody exercises at runtime, and have the new test only
check we do not segfault on a value we used to segfault upon seeing.

On Fri, Mar 28, 2014 at 12:30:02PM -0700, Junio C Hamano wrote:

Let's just deal with a simple known cases (like FreeBSD) in the real
code that everybody exercises at runtime, and have the new test only
check we do not segfault on a value we used to segfault upon seeing.

OK. Here it is, with the other option as an "alt" patch for reference.

  [1/2]: date: recognize bogus FreeBSD gmtime output
  [2/2]: t4212: loosen far-in-future test for AIX
  [2alt/2]: work around unreliable gmtime errors on AIX

-Peff

Most gmtime implementations return a NULL value when they
encounter an error (and this behavior is specified by ANSI C
and POSIX).  FreeBSD's implementation, however, will
indicate an error by returning a pointer to a "struct tm"
with all fields set to zero. Let's also recognize this and
convert it to a NULL (with this patch, t4212 should pass on
FreeBSD).

Reported-by: René Scharfe <redacted>
Signed-off-by: Jeff King <redacted>
---
There are actually a few callers to gmtime and gmtime_r, so I pushed
this fix up into a compat wrapper rather than in time_to_tm to get them
all. It's possible that localtime() would want to receive the same
treatment, too.  It's not strictly necessary to make the wrapper
conditional, but it was easy to do so. We could also just run this code
all the time.

I don't have a FreeBSD VM handy to test this, so confirmation that it
passes the test would be nice.

 Makefile          |  8 ++++++++
 compat/gmtime.c   | 26 ++++++++++++++++++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  7 +++++++
 4 files changed, 42 insertions(+)
 create mode 100644 compat/gmtime.c

diff --git a/Makefile b/Makefile
index 3646391..2f3758c 100644
--- a/Makefile
+++ b/Makefile

@@ -338,6 +338,9 @@ all::
 # Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite
 # with a different indexfile format version.  If it isn't set the index
 # file format used is index-v[23].
+#
+# Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
+# return NULL when it receives a bogus time_t.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -1489,6 +1492,11 @@ ifneq (,$(XDL_FAST_HASH))
 	BASIC_CFLAGS += -DXDL_FAST_HASH
 endif
 
+ifdef GMTIME_UNRELIABLE_ERRORS
+	COMPAT_OBJS += compat/gmtime.o
+	BASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

diff --git a/compat/gmtime.c b/compat/gmtime.c
new file mode 100644
index 0000000..ffcabf4
--- /dev/null
+++ b/compat/gmtime.c

@@ -0,0 +1,26 @@
+#include "../git-compat-util.h"
+#undef gmtime
+#undef gmtime_r
+
+struct tm *git_gmtime(const time_t *timep)
+{
+	static struct tm result;
+	return git_gmtime_r(timep, &result);
+}
+
+struct tm *git_gmtime_r(const time_t *timep, struct tm *result)
+{
+	struct tm *ret;
+
+	ret = gmtime_r(timep, result);
+
+	/*
+	 * Rather than NULL, FreeBSD gmtime will return a "struct tm" with all
+	 * fields zeroed. Since "mday" cannot otherwise be zero, we can test
+	 * this very quickly.
+	 */
+	if (ret && !ret->tm_mday)
+		ret = NULL;
+
+	return ret;
+}

diff --git a/config.mak.uname b/config.mak.uname
index 6069a44..0e22ac0 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -187,6 +187,7 @@ ifeq ($(uname_S),FreeBSD)
 	endif
 	PYTHON_PATH = /usr/local/bin/python
 	HAVE_PATHS_H = YesPlease
+	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease

diff --git a/git-compat-util.h b/git-compat-util.h
index 892032b..5191866 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -716,4 +716,11 @@ void warn_on_inaccessible(const char *path);
 /* Get the passwd entry for the UID of the current process. */
 struct passwd *xgetpwuid_self(void);
 
+#ifdef GMTIME_UNRELIABLE_ERRORS
+struct tm *git_gmtime(const time_t *);
+struct tm *git_gmtime_r(const time_t *, struct tm *);
+#define gmtime git_gmtime
+#define gmtime_r git_gmtime_r
+#endif
+
 #endif

-- 
1.9.1.656.ge8a0637

One of the tests in t4212 checks our behavior when we feed
gmtime a date so far in the future that it gives up and
returns NULL. Some implementations, like AIX, may actually
just provide us a bogus result instead.

It's not worth it for us to come up with heuristics that
guess whether the return value is sensible or not. On good
platforms where gmtime reports the problem to us with NULL,
we will print the epoch value. On bad platforms, we will
print garbage.  But our test should be written for the
lowest common denominator so that it passes everywhere.

Reported-by: Charles Bailey <redacted>
Signed-off-by: Jeff King <redacted>
---
 t/t4212-log-corrupt.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
index 3fa1715..58b792b 100755
--- a/t/t4212-log-corrupt.sh
+++ b/t/t4212-log-corrupt.sh

@@ -82,11 +82,9 @@ test_expect_success 'date parser recognizes time_t overflow' '
 '
 
 # date is within 2^63-1, but enough to choke glibc's gmtime
-test_expect_success 'absurdly far-in-future dates produce sentinel' '
+test_expect_success 'absurdly far-in-future date' '
 	commit=$(munge_author_date HEAD 999999999999999999) &&
-	echo "Thu Jan 1 00:00:00 1970 +0000" >expect &&
-	git log -1 --format=%ad $commit >actual &&
-	test_cmp expect actual
+	git log -1 --format=%ad $commit
 '
 
 test_done

-- 
1.9.1.656.ge8a0637

AIX's gmtime will happily overflow the tm_year field. Let's
catch this error before handing the value to gmtime.

Signed-off-by: Jeff King <redacted>
---
This is an alternative to loosening the test in t4212.

It's really not _that_ ugly.  The "LL" here may not be portable, though.
32-bit systems can't represent this timestamp at all (so they're safe),
but I don't know what would be the best way to conditionally compile
here.

 compat/gmtime.c  | 10 ++++++++++
 config.mak.uname |  1 +
 2 files changed, 11 insertions(+)

diff --git a/compat/gmtime.c b/compat/gmtime.c
index 75a5835..f95ba50 100644
--- a/compat/gmtime.c
+++ b/compat/gmtime.c

@@ -12,6 +12,16 @@ struct tm *git_gmtime_r(const time_t *timep, struct tm *result)
 {
 	struct tm *ret;
 
+	/*
+	 * Some systems, like AIX, will happily overflow the tm_year field.
+	 * So let's recognize obviously out-of-bound data before it hits gmtime
+	 * and just mark it as an error. This date is ~316 million years in the
+	 * future, which is far enough that nobody should care, but close
+	 * enough for the year to fit into a 32-bit tm_year.
+	 */
+	if (*timep > 9999999999999999LL)
+		return NULL;
+
 	ret = gmtime_r(timep, result);
 
 	/*

diff --git a/config.mak.uname b/config.mak.uname
index 0e22ac0..c1110ad 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -236,6 +236,7 @@ ifeq ($(uname_S),AIX)
 		INLINE = ''
 	endif
 	GIT_TEST_CMP = cmp
+	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 endif
 ifeq ($(uname_S),GNU)
 	# GNU/Hurd

-- 
1.9.1.656.ge8a0637

Am 01.04.2014 09:42, schrieb Jeff King:

http://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html 
says that errno shall be set on error and only mentions EOVERFLOW as a 
possible error code.

René