Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output
On Tue, Apr 01, 2014 at 11:17:14PM +0200, René Scharfe wrote: > >So are you saying we should set EOVERFLOW ourselves, or does FreeBSD > >set EOVERFLOW for us in this case and we do not have to worry? > > If we correct the return value then we should correct errno as well. > gmtime() on FreeBSD 10 leaves errno untouched when it encounters an > overflow. > > While testing this again I just noticed that FreeBSD doesn't strictly return > a pointer to a cleared struct tm. It simply leaves its static buffer > untouched. We should probably clear it (memset in git_gmtime_r). Thanks, that all makes sense (we do not ever care about gmtime's errno in our code, but it does not hurt to be thorough to avoid surprising any new callers). Here's a replacement for patch 1. -- >8 -- Subject: date: recognize bogus FreeBSD gmtime output 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 simply leave the "struct tm" untouched. Let's also recognize this and convert it to a NULL (with this patch, t4212 should pass on FreeBSD). Reported-by: René Scharfe Signed-off-by: Jeff King --- Makefile | 8 compat/gmtime.c | 29 + config.mak.uname | 1 + git-compat-util.h | 7 +++ 4 files changed, 45 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 000..e8362dd --- /dev/null +++ b/compat/gmtime.c @@ -0,0 +1,29 @@ +#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; + + memset(result, 0, sizeof(*result)); + ret = gmtime_r(timep, result); + + /* +* Rather than NULL, FreeBSD gmtime simply leaves the "struct tm" +* untouched when it encounters overflow. Since "mday" cannot otherwise +* be zero, we can test this very quickly. +*/ + if (ret && !ret->tm_mday) { + ret = NULL; + errno = EOVERFLOW; + } + + 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output
Am 01.04.2014 21:08, schrieb Junio C Hamano: René Scharfe writes: Am 01.04.2014 09:42, schrieb Jeff King: diff --git a/compat/gmtime.c b/compat/gmtime.c new file mode 100644 index 000..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; +} 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. So are you saying we should set EOVERFLOW ourselves, or does FreeBSD set EOVERFLOW for us in this case and we do not have to worry? If we correct the return value then we should correct errno as well. gmtime() on FreeBSD 10 leaves errno untouched when it encounters an overflow. While testing this again I just noticed that FreeBSD doesn't strictly return a pointer to a cleared struct tm. It simply leaves its static buffer untouched. We should probably clear it (memset in git_gmtime_r). Demo program: -- >8 -- #include #include const static time_t epoch; int main(int argc, char **argv) { time_t t = 9; printf("%s", ctime(&t)); printf("%s", asctime(gmtime(&t))); printf("%s", ctime(&t)); gmtime(&epoch); printf("%s", asctime(gmtime(&t))); return 0; } -- 8< -- Results on FreeBSD 10: Wed Sep 6 11:46:39 -1126091476 Wed Sep 6 11:46:39 -1126091476 Wed Sep 6 11:46:39 -1126091476 Thu Jan 1 00:00:00 1970 René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output
René Scharfe writes: > Am 01.04.2014 09:42, schrieb Jeff King: >> diff --git a/compat/gmtime.c b/compat/gmtime.c >> new file mode 100644 >> index 000..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; >> +} > > 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. So are you saying we should set EOVERFLOW ourselves, or does FreeBSD set EOVERFLOW for us in this case and we do not have to worry? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] date: recognize bogus FreeBSD gmtime output
Am 01.04.2014 09:42, schrieb Jeff King: diff --git a/compat/gmtime.c b/compat/gmtime.c new file mode 100644 index 000..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; +} 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é -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] date: recognize bogus FreeBSD gmtime output
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 Signed-off-by: Jeff King --- 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 000..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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html