[PATCH 0/5] drop non-reentrant time usage

[PATCH 0/5] drop non-reentrant time usage

[Doan Tran Cong Danh]

gmtime/localtime is considered unsafe in multithread environment.

git was started as single-thread application, but we have some
multi-thread code, right now.

replace all usage of gmtime/localtime by their respective reentrant ones.

On Linux (with both glibc and musl), we won't notice any differences in
performance, since they implemented gmtime and gmtime_r mostly the same.

On Windows, we may be taxed, since gmtime_r and localtime_r is our compat
functions, because we memcpy from returned data of gmtime/localtime.

To address that, I made patch #5 and included it together with this series.
I'm not sure how much portable it is.
It seems to run fine with my Windows 7 x86 and Windows 10 x64 VM, (inside
git-sdk for Windows, if it matters).
I'm Cc-ing j6t and Dscho on that patch to get their opinions.


Doan Tran Cong Danh (5):
  date.c::datestamp: switch to reentrant localtime_r
  date.c::time_to_tm_local: use reentrant localtime_r(3)
  date.c::time_to_tm: use reentrant gmtime_r(3)
  archive-zip: use reentrant localtime_r(3)
  mingw: use {gm,local}time_s as backend for {gm,local}time_r

 archive-zip.c  | 10 +++++-----
 compat/mingw.c | 13 +++++++------
 date.c         | 18 ++++++++++--------
 3 files changed, 22 insertions(+), 19 deletions(-)

-- 
2.24.0.158.gd77a74f4dd.dirty

[PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r

[Doan Tran Cong Danh]

Originally, git was intended to be single-thread executable.
`localtime(3)' can be used in such codebase for cleaner code.

Overtime, we're employing multithread in our code base.

Let's phase out `localtime(3)' with the favour of `localtime_r(3)'
in this public interface.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 date.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index 041db7db4e..553f8e79a2 100644
--- a/date.c
+++ b/date.c
@@ -959,10 +959,11 @@ void datestamp(struct strbuf *out)
 {
 	time_t now;
 	int offset;
+	struct tm tm = { 0 };
 
 	time(&now);
 
-	offset = tm_to_time_t(localtime(&now)) - now;
+	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
 	offset /= 60;
 
 	date_string(now, offset, out);
-- 
2.24.0.158.gd77a74f4dd.dirty

[PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3)

[Doan Tran Cong Danh]

Phase out `localtime(3)' in favour of reentrant `localtime_r(3)'

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 date.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/date.c b/date.c
index 553f8e79a2..ca71736a9f 100644
--- a/date.c
+++ b/date.c
@@ -70,10 +70,10 @@ static struct tm *time_to_tm(timestamp_t time, int tz)
 	return gmtime(&t);
 }
 
-static struct tm *time_to_tm_local(timestamp_t time)
+static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)
 {
 	time_t t = time;
-	return localtime(&t);
+	return localtime_r(&t, tm);
 }
 
 /*
@@ -283,6 +283,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
 const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
+	struct tm tmbuf = { 0 };
 	struct tm human_tm = { 0 };
 	int human_tz = -1;
 	static struct strbuf timebuf = STRBUF_INIT;
@@ -318,7 +319,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 	}
 
 	if (mode->local)
-		tm = time_to_tm_local(time);
+		tm = time_to_tm_local(time, &tmbuf);
 	else
 		tm = time_to_tm(time, tz);
 	if (!tm) {
-- 
2.24.0.158.gd77a74f4dd.dirty

[PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3)

[Doan Tran Cong Danh]

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 date.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/date.c b/date.c
index ca71736a9f..b0d9a8421d 100644
--- a/date.c
+++ b/date.c
@@ -64,10 +64,10 @@ static time_t gm_time_t(timestamp_t time, int tz)
  * thing, which means that tz -0100 is passed in as the integer -100,
  * even though it means "sixty minutes off"
  */
-static struct tm *time_to_tm(timestamp_t time, int tz)
+static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)
 {
 	time_t t = gm_time_t(time, tz);
-	return gmtime(&t);
+	return gmtime_r(&t, tm);
 }
 
 static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)
@@ -321,9 +321,9 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 	if (mode->local)
 		tm = time_to_tm_local(time, &tmbuf);
 	else
-		tm = time_to_tm(time, tz);
+		tm = time_to_tm(time, tz, &tmbuf);
 	if (!tm) {
-		tm = time_to_tm(0, 0);
+		tm = time_to_tm(0, 0, &tmbuf);
 		tz = 0;
 	}
 
-- 
2.24.0.158.gd77a74f4dd.dirty

[PATCH 4/5] archive-zip: use reentrant localtime_r(3)

[Doan Tran Cong Danh]

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 archive-zip.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 4d66b5be6e..8bbd3e5884 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -603,18 +603,18 @@ static void write_zip_trailer(const struct object_id *oid)
 static void dos_time(timestamp_t *timestamp, int *dos_date, int *dos_time)
 {
 	time_t time;
-	struct tm *t;
+	struct tm tm;
 
 	if (date_overflows(*timestamp))
 		die(_("timestamp too large for this system: %"PRItime),
 		    *timestamp);
 	time = (time_t)*timestamp;
-	t = localtime(&time);
+	localtime_r(&time, &tm);
 	*timestamp = time;
 
-	*dos_date = t->tm_mday + (t->tm_mon + 1) * 32 +
-	            (t->tm_year + 1900 - 1980) * 512;
-	*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
+	*dos_date = tm.tm_mday + (tm.tm_mon + 1) * 32 +
+	            (tm.tm_year + 1900 - 1980) * 512;
+	*dos_time = tm.tm_sec / 2 + tm.tm_min * 32 + tm.tm_hour * 2048;
 }
 
 static int archive_zip_config(const char *var, const char *value, void *data)
-- 
2.24.0.158.gd77a74f4dd.dirty

[PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r

[Doan Tran Cong Danh]

Since Windows doesn't provide gmtime_r(3) and localtime_r(3),
we're providing a compat version by using non-reentrant gmtime(3) and
localtime(3) as backend. Then, we copy the returned data into the
buffer.

By doing that, in case of failure, we will dereference a NULL pointer
returned by gmtime(3), and localtime(3), and we always return a valid
pointer instead of NULL.

Drop the memcpy(3) by using gmtime_s, and localtime_s as backend on
Windows, and make sure we will return NULL in case of failure.

Cc: Johannes Sixt <j6t@kdbg.org>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 compat/mingw.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index fe609239dd..7b21f4eee5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1,6 +1,7 @@
 #include "../git-compat-util.h"
 #include "win32.h"
 #include <conio.h>
+#include <errno.h>
 #include <wchar.h>
 #include "../strbuf.h"
 #include "../run-command.h"
@@ -986,16 +987,16 @@ int pipe(int filedes[2])
 
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
-	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
-	memcpy(result, gmtime(timep), sizeof(struct tm));
-	return result;
+	if (gmtime_s(result, timep) == 0)
+		return result;
+	return NULL;
 }
 
 struct tm *localtime_r(const time_t *timep, struct tm *result)
 {
-	/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */
-	memcpy(result, localtime(timep), sizeof(struct tm));
-	return result;
+	if (localtime_s(result, timep) == 0)
+		return result;
+	return NULL;
 }
 
 char *mingw_getcwd(char *pointer, int len)
-- 
2.24.0.158.gd77a74f4dd.dirty

Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r

[Johannes Schindelin]

Hi Danh,

On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:

> Since Windows doesn't provide gmtime_r(3) and localtime_r(3),
> we're providing a compat version by using non-reentrant gmtime(3) and
> localtime(3) as backend. Then, we copy the returned data into the
> buffer.
>
> By doing that, in case of failure, we will dereference a NULL pointer
> returned by gmtime(3), and localtime(3), and we always return a valid
> pointer instead of NULL.
>
> Drop the memcpy(3) by using gmtime_s, and localtime_s as backend on

s/and localtime_s/and use localtime_s()/

Otherwise, this looks good to me, thank you!

Ciao,
Dscho

> Windows, and make sure we will return NULL in case of failure.
>
> Cc: Johannes Sixt <j6t@kdbg.org>
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
> ---
>  compat/mingw.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index fe609239dd..7b21f4eee5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1,6 +1,7 @@
>  #include "../git-compat-util.h"
>  #include "win32.h"
>  #include <conio.h>
> +#include <errno.h>
>  #include <wchar.h>
>  #include "../strbuf.h"
>  #include "../run-command.h"
> @@ -986,16 +987,16 @@ int pipe(int filedes[2])
>
>  struct tm *gmtime_r(const time_t *timep, struct tm *result)
>  {
> -	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
> -	memcpy(result, gmtime(timep), sizeof(struct tm));
> -	return result;
> +	if (gmtime_s(result, timep) == 0)
> +		return result;
> +	return NULL;
>  }
>
>  struct tm *localtime_r(const time_t *timep, struct tm *result)
>  {
> -	/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */
> -	memcpy(result, localtime(timep), sizeof(struct tm));
> -	return result;
> +	if (localtime_s(result, timep) == 0)
> +		return result;
> +	return NULL;
>  }
>
>  char *mingw_getcwd(char *pointer, int len)
> --
> 2.24.0.158.gd77a74f4dd.dirty
>
>

Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r

[Johannes Schindelin]

Hi Danh,

On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:

> diff --git a/compat/mingw.c b/compat/mingw.c
> index fe609239dd..7b21f4eee5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1,6 +1,7 @@
>  #include "../git-compat-util.h"
>  #include "win32.h"
>  #include <conio.h>
> +#include <errno.h>

I actually overlooked this. Please drop this hunk, it should not be needed
(and might break things in the MSVC build).

Thanks,
Dscho

>  #include <wchar.h>
>  #include "../strbuf.h"
>  #include "../run-command.h"
> @@ -986,16 +987,16 @@ int pipe(int filedes[2])
>
>  struct tm *gmtime_r(const time_t *timep, struct tm *result)
>  {
> -	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
> -	memcpy(result, gmtime(timep), sizeof(struct tm));
> -	return result;
> +	if (gmtime_s(result, timep) == 0)
> +		return result;
> +	return NULL;
>  }
>
>  struct tm *localtime_r(const time_t *timep, struct tm *result)
>  {
> -	/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */
> -	memcpy(result, localtime(timep), sizeof(struct tm));
> -	return result;
> +	if (localtime_s(result, timep) == 0)
> +		return result;
> +	return NULL;
>  }
>
>  char *mingw_getcwd(char *pointer, int len)
> --
> 2.24.0.158.gd77a74f4dd.dirty
>
>

Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r

[Danh Doan]

On 2019-11-27 20:39:27+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Danh,
> 
> On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:
> 
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index fe609239dd..7b21f4eee5 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -1,6 +1,7 @@
> >  #include "../git-compat-util.h"
> >  #include "win32.h"
> >  #include <conio.h>
> > +#include <errno.h>
> 
> I actually overlooked this. Please drop this hunk, it should not be needed
> (and might break things in the MSVC build).

Oops, originally, I intended to reset errno after {gm,local}time_s
Found out it's unneccessary later but I forgot to drop this hunk.
I'll fix it.

-- 
Danh

Re: [PATCH 0/5] drop non-reentrant time usage

[Jeff King]

On Wed, Nov 27, 2019 at 10:13:16PM +0700, Doan Tran Cong Danh wrote:

> gmtime/localtime is considered unsafe in multithread environment.
> 
> git was started as single-thread application, but we have some
> multi-thread code, right now.
> 
> replace all usage of gmtime/localtime by their respective reentrant ones.

I think this is a good change.

A minor point, but I think it may be simpler if the first four were just
a single patch. There's no rationale given at all in the 3rd and 4th
ones. Which is because you already explained it in patch 1, but that
won't help somebody who digs up the commit via "git blame".

So I think they either ought to be one patch, or they should repeat the
rationale (I'd probably go with the first, but I could live with the
second).

> On Windows, we may be taxed, since gmtime_r and localtime_r is our compat
> functions, because we memcpy from returned data of gmtime/localtime.
> 
> To address that, I made patch #5 and included it together with this series.
> I'm not sure how much portable it is.
> It seems to run fine with my Windows 7 x86 and Windows 10 x64 VM, (inside
> git-sdk for Windows, if it matters).
> I'm Cc-ing j6t and Dscho on that patch to get their opinions.

That makes sense from my reading of gmtime_s(), but there may be
something more subtle going on with compatibility, so I'll leave that to
the experts.

-Peff

Re: [PATCH 0/5] drop non-reentrant time usage

[Danh Doan]

On 2019-11-27 11:29:30-0500, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 27, 2019 at 10:13:16PM +0700, Doan Tran Cong Danh wrote:
> 
> > gmtime/localtime is considered unsafe in multithread environment.
> > 
> > git was started as single-thread application, but we have some
> > multi-thread code, right now.
> > 
> > replace all usage of gmtime/localtime by their respective reentrant ones.
> 
> I think this is a good change.
> 
> A minor point, but I think it may be simpler if the first four were just
> a single patch. There's no rationale given at all in the 3rd and 4th
> ones. Which is because you already explained it in patch 1, but that
> won't help somebody who digs up the commit via "git blame".
> 
> So I think they either ought to be one patch, or they should repeat the
> rationale (I'd probably go with the first, but I could live with the
> second).

I'll merge first three into one since both of them are in date.c,
the 4th one changed another file.

-- 
Danh

[PATCH v2 0/3] Phase out non-reentrant time functions

[Doan Tran Cong Danh]

gmtime/localtime is considered unsafe in multithread environment.

git was started as single-thread application, but we have some
multi-thread code, right now.

replace all usage of gmtime/localtime by their respective reentrant ones.

On most POSIX systems, we won't notice any differences in performance,
since they implemented gmtime and gmtime_r mostly the same.

On Windows, we may be taxed, since gmtime_r and localtime_r is our compat
functions, because we memcpy from returned data of gmtime/localtime.

To address that, I made patch #3 and included it together with this series.
I'm not sure how much portable it is.


Doan Tran Cong Danh (3):
  date.c: switch to reentrant {gm,local}time_r
  archive-zip.c: switch to reentrant localtime_r
  mingw: use {gm,local}time_s as backend for {gm,local}time_r

 archive-zip.c  | 10 +++++-----
 compat/mingw.c | 12 ++++++------
 date.c         | 18 ++++++++++--------
 3 files changed, 21 insertions(+), 19 deletions(-)

Range-diff against v1:
1:  1e7e08b3c7 < -:  ---------- date.c::datestamp: switch to reentrant localtime_r
2:  130eba77db ! 1:  71de738864 date.c::time_to_tm_local: use reentrant localtime_r(3)
    @@ Metadata
     Author: Doan Tran Cong Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    date.c::time_to_tm_local: use reentrant localtime_r(3)
    +    date.c: switch to reentrant {gm,local}time_r
     
    -    Phase out `localtime(3)' in favour of reentrant `localtime_r(3)'
    +    Originally, git was intended to be single-thread executable.
    +    `gmtime(3)' and `localtime(3)' can be used in such codebase
    +    for cleaner code.
    +
    +    Overtime, we're employing multithread in our code base.
    +
    +    Let's phase out `gmtime(3)' and `localtime(3)' in favour of
    +    `gmtime_r(3)' and `localtime_r(3)'.
     
         Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
     
      ## date.c ##
    -@@ date.c: static struct tm *time_to_tm(timestamp_t time, int tz)
    - 	return gmtime(&t);
    +@@ date.c: static time_t gm_time_t(timestamp_t time, int tz)
    +  * thing, which means that tz -0100 is passed in as the integer -100,
    +  * even though it means "sixty minutes off"
    +  */
    +-static struct tm *time_to_tm(timestamp_t time, int tz)
    ++static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)
    + {
    + 	time_t t = gm_time_t(time, tz);
    +-	return gmtime(&t);
    ++	return gmtime_r(&t, tm);
      }
      
     -static struct tm *time_to_tm_local(timestamp_t time)
    @@ date.c: const char *show_date(timestamp_t time, int tz, const struct date_mode *
     -		tm = time_to_tm_local(time);
     +		tm = time_to_tm_local(time, &tmbuf);
      	else
    - 		tm = time_to_tm(time, tz);
    +-		tm = time_to_tm(time, tz);
    ++		tm = time_to_tm(time, tz, &tmbuf);
      	if (!tm) {
    +-		tm = time_to_tm(0, 0);
    ++		tm = time_to_tm(0, 0, &tmbuf);
    + 		tz = 0;
    + 	}
    + 
    +@@ date.c: void datestamp(struct strbuf *out)
    + {
    + 	time_t now;
    + 	int offset;
    ++	struct tm tm = { 0 };
    + 
    + 	time(&now);
    + 
    +-	offset = tm_to_time_t(localtime(&now)) - now;
    ++	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
    + 	offset /= 60;
    + 
    + 	date_string(now, offset, out);
3:  ce7fe7bcf3 < -:  ---------- date.c::time_to_tm: use reentrant gmtime_r(3)
4:  f6fd89dfe1 ! 2:  77798f1773 archive-zip: use reentrant localtime_r(3)
    @@ Metadata
     Author: Doan Tran Cong Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    archive-zip: use reentrant localtime_r(3)
    +    archive-zip.c: switch to reentrant localtime_r
    +
    +    Originally, git was intended to be single-thread executable.
    +    `localtime(3)' can be used in such codebase for cleaner code.
    +
    +    Overtime, we're employing multithread in our code base.
    +
    +    Let's phase out `gmtime(3)' in favour of `localtime_r(3)'.
    +
    +    Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
     
      ## archive-zip.c ##
     @@ archive-zip.c: static void write_zip_trailer(const struct object_id *oid)
5:  2c39f9a04f ! 3:  33a67eb377 mingw: use {gm,local}time_s as backend for {gm,local}time_r
    @@ Commit message
         Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
     
      ## compat/mingw.c ##
    -@@
    - #include "../git-compat-util.h"
    - #include "win32.h"
    - #include <conio.h>
    -+#include <errno.h>
    - #include <wchar.h>
    - #include "../strbuf.h"
    - #include "../run-command.h"
     @@ compat/mingw.c: int pipe(int filedes[2])
      
      struct tm *gmtime_r(const time_t *timep, struct tm *result)
-- 
2.24.0.615.g37f5bfbdea

[PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r

[Doan Tran Cong Danh]

Originally, git was intended to be single-thread executable.
`gmtime(3)' and `localtime(3)' can be used in such codebase
for cleaner code.

Overtime, we're employing multithread in our code base.

Let's phase out `gmtime(3)' and `localtime(3)' in favour of
`gmtime_r(3)' and `localtime_r(3)'.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 date.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/date.c b/date.c
index 041db7db4e..b0d9a8421d 100644
--- a/date.c
+++ b/date.c
@@ -64,16 +64,16 @@ static time_t gm_time_t(timestamp_t time, int tz)
  * thing, which means that tz -0100 is passed in as the integer -100,
  * even though it means "sixty minutes off"
  */
-static struct tm *time_to_tm(timestamp_t time, int tz)
+static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)
 {
 	time_t t = gm_time_t(time, tz);
-	return gmtime(&t);
+	return gmtime_r(&t, tm);
 }
 
-static struct tm *time_to_tm_local(timestamp_t time)
+static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)
 {
 	time_t t = time;
-	return localtime(&t);
+	return localtime_r(&t, tm);
 }
 
 /*
@@ -283,6 +283,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm
 const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 {
 	struct tm *tm;
+	struct tm tmbuf = { 0 };
 	struct tm human_tm = { 0 };
 	int human_tz = -1;
 	static struct strbuf timebuf = STRBUF_INIT;
@@ -318,11 +319,11 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 	}
 
 	if (mode->local)
-		tm = time_to_tm_local(time);
+		tm = time_to_tm_local(time, &tmbuf);
 	else
-		tm = time_to_tm(time, tz);
+		tm = time_to_tm(time, tz, &tmbuf);
 	if (!tm) {
-		tm = time_to_tm(0, 0);
+		tm = time_to_tm(0, 0, &tmbuf);
 		tz = 0;
 	}
 
@@ -959,10 +960,11 @@ void datestamp(struct strbuf *out)
 {
 	time_t now;
 	int offset;
+	struct tm tm = { 0 };
 
 	time(&now);
 
-	offset = tm_to_time_t(localtime(&now)) - now;
+	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
 	offset /= 60;
 
 	date_string(now, offset, out);
-- 
2.24.0.615.g37f5bfbdea

[PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r

[Doan Tran Cong Danh]

Originally, git was intended to be single-thread executable.
`localtime(3)' can be used in such codebase for cleaner code.

Overtime, we're employing multithread in our code base.

Let's phase out `gmtime(3)' in favour of `localtime_r(3)'.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 archive-zip.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 4d66b5be6e..8bbd3e5884 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -603,18 +603,18 @@ static void write_zip_trailer(const struct object_id *oid)
 static void dos_time(timestamp_t *timestamp, int *dos_date, int *dos_time)
 {
 	time_t time;
-	struct tm *t;
+	struct tm tm;
 
 	if (date_overflows(*timestamp))
 		die(_("timestamp too large for this system: %"PRItime),
 		    *timestamp);
 	time = (time_t)*timestamp;
-	t = localtime(&time);
+	localtime_r(&time, &tm);
 	*timestamp = time;
 
-	*dos_date = t->tm_mday + (t->tm_mon + 1) * 32 +
-	            (t->tm_year + 1900 - 1980) * 512;
-	*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
+	*dos_date = tm.tm_mday + (tm.tm_mon + 1) * 32 +
+	            (tm.tm_year + 1900 - 1980) * 512;
+	*dos_time = tm.tm_sec / 2 + tm.tm_min * 32 + tm.tm_hour * 2048;
 }
 
 static int archive_zip_config(const char *var, const char *value, void *data)
-- 
2.24.0.615.g37f5bfbdea

[PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r

[Doan Tran Cong Danh]

Since Windows doesn't provide gmtime_r(3) and localtime_r(3),
we're providing a compat version by using non-reentrant gmtime(3) and
localtime(3) as backend. Then, we copy the returned data into the
buffer.

By doing that, in case of failure, we will dereference a NULL pointer
returned by gmtime(3), and localtime(3), and we always return a valid
pointer instead of NULL.

Drop the memcpy(3) by using gmtime_s, and localtime_s as backend on
Windows, and make sure we will return NULL in case of failure.

Cc: Johannes Sixt <j6t@kdbg.org>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 compat/mingw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index fe609239dd..75695a24a3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -986,16 +986,16 @@ int pipe(int filedes[2])
 
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
-	/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */
-	memcpy(result, gmtime(timep), sizeof(struct tm));
-	return result;
+	if (gmtime_s(result, timep) == 0)
+		return result;
+	return NULL;
 }
 
 struct tm *localtime_r(const time_t *timep, struct tm *result)
 {
-	/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */
-	memcpy(result, localtime(timep), sizeof(struct tm));
-	return result;
+	if (localtime_s(result, timep) == 0)
+		return result;
+	return NULL;
 }
 
 char *mingw_getcwd(char *pointer, int len)
-- 
2.24.0.615.g37f5bfbdea