threads / patch / 52359

patch, 5 partsdrop non-reentrant time usage

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

## tl;dr

15 messages between Nov 27, 2019 and Nov 28, 2019. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · lore
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
Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · re: Doan Tran Cong Danh · lore

[PATCH 1/5] date.c::datestamp: 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 `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(-)
Show changes to date.c +2 −1
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
Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · re: Doan Tran Cong Danh · lore

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

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(-)
Show changes to date.c +4 −3
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
Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · re: Doan Tran Cong Danh · lore

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

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 date.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to date.c +4 −4
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
Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · re: Doan Tran Cong Danh · lore

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

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---
 archive-zip.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to archive-zip.c +5 −5
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
Doan Tran Cong Danh· Nov 27, 2019, 15:13 UTC · re: Doan Tran Cong Danh · lore

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

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(-)
Show changes to compat/mingw.c +7 −6
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
Johannes Schindelin· Nov 27, 2019, 19:35 UTC · re: Doan Tran Cong Danh · lore

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

Hi Danh,
On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:
Show 10 quoted lines
> 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

Show 48 quoted lines
> 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
>
>
Johannes Schindelin· Nov 27, 2019, 19:39 UTC · re: Doan Tran Cong Danh · lore

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

Hi Danh,
On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:
Show 9 quoted lines
> 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

Show 30 quoted lines
>  #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
>
>
Danh Doan· Nov 28, 2019, 12:05 UTC · re: Johannes Schindelin · lore

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

On 2019-11-27 20:39:27+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 16 quoted lines
> 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
Jeff King· Nov 27, 2019, 16:29 UTC · re: Doan Tran Cong Danh · lore

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

On Wed, Nov 27, 2019 at 10:13:16PM +0700, Doan Tran Cong Danh wrote:
Show 6 quoted lines
> 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).

Show 8 quoted lines
> 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
Danh Doan· Nov 28, 2019, 12:16 UTC · re: Jeff King · lore

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

On 2019-11-27 11:29:30-0500, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> 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
Doan Tran Cong Danh· Nov 28, 2019, 12:25 UTC · re: Doan Tran Cong Danh · lore

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

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
Doan Tran Cong Danh· Nov 28, 2019, 12:25 UTC · re: Doan Tran Cong Danh · lore

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

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(-)
Show changes to date.c +10 −8
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
Doan Tran Cong Danh· Nov 28, 2019, 12:25 UTC · re: Doan Tran Cong Danh · lore

[PATCH v2 2/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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to archive-zip.c +5 −5
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
Doan Tran Cong Danh· Nov 28, 2019, 12:25 UTC · re: Doan Tran Cong Danh · lore

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

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(-)
Show changes to compat/mingw.c +6 −6
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

← back to recent threads