Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] drop non-reentrant time usage
@ 2019-11-27 15:13 Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r Doan Tran Cong Danh
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
@ 2019-11-27 15:13 ` Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3) Doan Tran Cong Danh
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3)
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r Doan Tran Cong Danh
@ 2019-11-27 15:13 ` Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3) Doan Tran Cong Danh
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3)
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3) Doan Tran Cong Danh
@ 2019-11-27 15:13 ` Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 4/5] archive-zip: use reentrant localtime_r(3) Doan Tran Cong Danh
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/5] archive-zip: use reentrant localtime_r(3)
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
                   ` (2 preceding siblings ...)
  2019-11-27 15:13 ` [PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3) Doan Tran Cong Danh
@ 2019-11-27 15:13 ` Doan Tran Cong Danh
  2019-11-27 15:13 ` [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
                   ` (3 preceding siblings ...)
  2019-11-27 15:13 ` [PATCH 4/5] archive-zip: use reentrant localtime_r(3) Doan Tran Cong Danh
@ 2019-11-27 15:13 ` Doan Tran Cong Danh
  2019-11-27 19:35   ` Johannes Schindelin
  2019-11-27 19:39   ` Johannes Schindelin
  2019-11-27 16:29 ` [PATCH 0/5] drop non-reentrant time usage Jeff King
  2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
  6 siblings, 2 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-27 15:13 UTC (permalink / raw)
  To: git; +Cc: Doan Tran Cong Danh, Johannes Sixt, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] drop non-reentrant time usage
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
                   ` (4 preceding siblings ...)
  2019-11-27 15:13 ` [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
@ 2019-11-27 16:29 ` Jeff King
  2019-11-28 12:16   ` Danh Doan
  2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
  6 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2019-11-27 16:29 UTC (permalink / raw)
  To: Doan Tran Cong Danh; +Cc: git

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r
  2019-11-27 15:13 ` [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
@ 2019-11-27 19:35   ` Johannes Schindelin
  2019-11-27 19:39   ` Johannes Schindelin
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Schindelin @ 2019-11-27 19:35 UTC (permalink / raw)
  To: Doan Tran Cong Danh; +Cc: git, Johannes Sixt

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
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r
  2019-11-27 15:13 ` [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
  2019-11-27 19:35   ` Johannes Schindelin
@ 2019-11-27 19:39   ` Johannes Schindelin
  2019-11-28 12:05     ` Danh Doan
  1 sibling, 1 reply; 15+ messages in thread
From: Johannes Schindelin @ 2019-11-27 19:39 UTC (permalink / raw)
  To: Doan Tran Cong Danh; +Cc: git, Johannes Sixt

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
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r
  2019-11-27 19:39   ` Johannes Schindelin
@ 2019-11-28 12:05     ` Danh Doan
  0 siblings, 0 replies; 15+ messages in thread
From: Danh Doan @ 2019-11-28 12:05 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] drop non-reentrant time usage
  2019-11-27 16:29 ` [PATCH 0/5] drop non-reentrant time usage Jeff King
@ 2019-11-28 12:16   ` Danh Doan
  0 siblings, 0 replies; 15+ messages in thread
From: Danh Doan @ 2019-11-28 12:16 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/3] Phase out non-reentrant time functions
  2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
                   ` (5 preceding siblings ...)
  2019-11-27 16:29 ` [PATCH 0/5] drop non-reentrant time usage Jeff King
@ 2019-11-28 12:25 ` Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r Doan Tran Cong Danh
                     ` (2 more replies)
  6 siblings, 3 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-28 12:25 UTC (permalink / raw)
  To: git; +Cc: peff, 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r
  2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
@ 2019-11-28 12:25   ` Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
  2 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-28 12:25 UTC (permalink / raw)
  To: git; +Cc: peff, 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r
  2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r Doan Tran Cong Danh
@ 2019-11-28 12:25   ` Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
  2 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-28 12:25 UTC (permalink / raw)
  To: git; +Cc: peff, 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r
  2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r Doan Tran Cong Danh
  2019-11-28 12:25   ` [PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r Doan Tran Cong Danh
@ 2019-11-28 12:25   ` Doan Tran Cong Danh
  2 siblings, 0 replies; 15+ messages in thread
From: Doan Tran Cong Danh @ 2019-11-28 12:25 UTC (permalink / raw)
  To: git; +Cc: peff, Doan Tran Cong Danh, Johannes Sixt, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27 15:13 [PATCH 0/5] drop non-reentrant time usage Doan Tran Cong Danh
2019-11-27 15:13 ` [PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r Doan Tran Cong Danh
2019-11-27 15:13 ` [PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3) Doan Tran Cong Danh
2019-11-27 15:13 ` [PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3) Doan Tran Cong Danh
2019-11-27 15:13 ` [PATCH 4/5] archive-zip: use reentrant localtime_r(3) Doan Tran Cong Danh
2019-11-27 15:13 ` [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh
2019-11-27 19:35   ` Johannes Schindelin
2019-11-27 19:39   ` Johannes Schindelin
2019-11-28 12:05     ` Danh Doan
2019-11-27 16:29 ` [PATCH 0/5] drop non-reentrant time usage Jeff King
2019-11-28 12:16   ` Danh Doan
2019-11-28 12:25 ` [PATCH v2 0/3] Phase out non-reentrant time functions Doan Tran Cong Danh
2019-11-28 12:25   ` [PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r Doan Tran Cong Danh
2019-11-28 12:25   ` [PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r Doan Tran Cong Danh
2019-11-28 12:25   ` [PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r Doan Tran Cong Danh

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git