{"thread":{"id":"52359","subject":"[PATCH 0/5] drop non-reentrant time usage","startedAt":"2019-11-27T15:13:27Z","lastAt":"2019-11-28T12:25:53Z","messageCount":15,"participants":["Doan Tran Cong Danh","Jeff King","Johannes Schindelin","Danh Doan"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"387161","messageId":"cover.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":null,"subject":"[PATCH 0/5] drop non-reentrant time usage","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:16Z","receivedAt":"2019-11-27T15:13:27Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"gmtime/localtime is considered unsafe in multithread environment.\n\ngit was started as single-thread application, but we have some\nmulti-thread code, right now.\n\nreplace all usage of gmtime/localtime by their respective reentrant ones.\n\nOn Linux (with both glibc and musl), we won't notice any differences in\nperformance, since they implemented gmtime and gmtime_r mostly the same.\n\nOn Windows, we may be taxed, since gmtime_r and localtime_r is our compat\nfunctions, because we memcpy from returned data of gmtime/localtime.\n\nTo address that, I made patch #5 and included it together with this series.\nI'm not sure how much portable it is.\nIt seems to run fine with my Windows 7 x86 and Windows 10 x64 VM, (inside\ngit-sdk for Windows, if it matters).\nI'm Cc-ing j6t and Dscho on that patch to get their opinions.\n\n\nDoan Tran Cong Danh (5):\n date.c::datestamp: switch to reentrant localtime_r\n date.c::time_to_tm_local: use reentrant localtime_r(3)\n date.c::time_to_tm: use reentrant gmtime_r(3)\n archive-zip: use reentrant localtime_r(3)\n mingw: use {gm,local}time_s as backend for {gm,local}time_r\n\n archive-zip.c | 10 +++++-----\n compat/mingw.c | 13 +++++++------\n date.c | 18 ++++++++++--------\n 3 files changed, 22 insertions(+), 19 deletions(-)\n\n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387162","messageId":"1e7e08b3c7a9d75d761b8ff63ddb291f6cca14e4.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH 1/5] date.c::datestamp: switch to reentrant localtime_r","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:17Z","receivedAt":"2019-11-27T15:13:38Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Originally, git was intended to be single-thread executable.\n`localtime(3)' can be used in such codebase for cleaner code.\n\nOvertime, we're employing multithread in our code base.\n\nLet's phase out `localtime(3)' with the favour of `localtime_r(3)'\nin this public interface.\n\nSigned-off-by: Doan Tran Cong Danh \n---\n date.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 041db7db4e..553f8e79a2 100644\n--- a/date.c\n+++ b/date.c\n@@ -959,10 +959,11 @@ void datestamp(struct strbuf *out)\n {\n \ttime_t now;\n \tint offset;\n+\tstruct tm tm = { 0 };\n \n \ttime(&now);\n \n-\toffset = tm_to_time_t(localtime(&now)) - now;\n+\toffset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n \toffset /= 60;\n \n \tdate_string(now, offset, out);\n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387163","messageId":"130eba77db3c953c53bd80c9721b4d0fe0f6bee0.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH 2/5] date.c::time_to_tm_local: use reentrant localtime_r(3)","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:18Z","receivedAt":"2019-11-27T15:13:40Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Phase out `localtime(3)' in favour of reentrant `localtime_r(3)'\n\nSigned-off-by: Doan Tran Cong Danh \n---\n date.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 553f8e79a2..ca71736a9f 100644\n--- a/date.c\n+++ b/date.c\n@@ -70,10 +70,10 @@ static struct tm *time_to_tm(timestamp_t time, int tz)\n \treturn gmtime(&t);\n }\n \n-static struct tm *time_to_tm_local(timestamp_t time)\n+static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)\n {\n \ttime_t t = time;\n-\treturn localtime(&t);\n+\treturn localtime_r(&t, tm);\n }\n \n /*\n@@ -283,6 +283,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n+\tstruct tm tmbuf = { 0 };\n \tstruct tm human_tm = { 0 };\n \tint human_tz = -1;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n@@ -318,7 +319,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t}\n \n \tif (mode->local)\n-\t\ttm = time_to_tm_local(time);\n+\t\ttm = time_to_tm_local(time, &tmbuf);\n \telse\n \t\ttm = time_to_tm(time, tz);\n \tif (!tm) {\n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387164","messageId":"ce7fe7bcf363826b8f67c91d7300eb0158091752.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH 3/5] date.c::time_to_tm: use reentrant gmtime_r(3)","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:19Z","receivedAt":"2019-11-27T15:13:45Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Signed-off-by: Doan Tran Cong Danh \n---\n date.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex ca71736a9f..b0d9a8421d 100644\n--- a/date.c\n+++ b/date.c\n@@ -64,10 +64,10 @@ static time_t gm_time_t(timestamp_t time, int tz)\n * thing, which means that tz -0100 is passed in as the integer -100,\n * even though it means \"sixty minutes off\"\n */\n-static struct tm *time_to_tm(timestamp_t time, int tz)\n+static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)\n {\n \ttime_t t = gm_time_t(time, tz);\n-\treturn gmtime(&t);\n+\treturn gmtime_r(&t, tm);\n }\n \n static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)\n@@ -321,9 +321,9 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \tif (mode->local)\n \t\ttm = time_to_tm_local(time, &tmbuf);\n \telse\n-\t\ttm = time_to_tm(time, tz);\n+\t\ttm = time_to_tm(time, tz, &tmbuf);\n \tif (!tm) {\n-\t\ttm = time_to_tm(0, 0);\n+\t\ttm = time_to_tm(0, 0, &tmbuf);\n \t\ttz = 0;\n \t}\n \n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387165","messageId":"f6fd89dfe12afbfddbdf718cb691025bb1541eef.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH 4/5] archive-zip: use reentrant localtime_r(3)","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:20Z","receivedAt":"2019-11-27T15:13:52Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Signed-off-by: Doan Tran Cong Danh \n---\n archive-zip.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 4d66b5be6e..8bbd3e5884 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -603,18 +603,18 @@ static void write_zip_trailer(const struct object_id *oid)\n static void dos_time(timestamp_t *timestamp, int *dos_date, int *dos_time)\n {\n \ttime_t time;\n-\tstruct tm *t;\n+\tstruct tm tm;\n \n \tif (date_overflows(*timestamp))\n \t\tdie(_(\"timestamp too large for this system: %\"PRItime),\n \t\t *timestamp);\n \ttime = (time_t)*timestamp;\n-\tt = localtime(&time);\n+\tlocaltime_r(&time, &tm);\n \t*timestamp = time;\n \n-\t*dos_date = t->tm_mday + (t->tm_mon + 1) * 32 +\n-\t (t->tm_year + 1900 - 1980) * 512;\n-\t*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;\n+\t*dos_date = tm.tm_mday + (tm.tm_mon + 1) * 32 +\n+\t (tm.tm_year + 1900 - 1980) * 512;\n+\t*dos_time = tm.tm_sec / 2 + tm.tm_min * 32 + tm.tm_hour * 2048;\n }\n \n static int archive_zip_config(const char *var, const char *value, void *data)\n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387166","messageId":"2c39f9a04f98fc3d365c80ce57e8edce305846fb.1574867409.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-27T15:13:21Z","receivedAt":"2019-11-27T15:14:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Since Windows doesn't provide gmtime_r(3) and localtime_r(3),\nwe're providing a compat version by using non-reentrant gmtime(3) and\nlocaltime(3) as backend. Then, we copy the returned data into the\nbuffer.\n\nBy doing that, in case of failure, we will dereference a NULL pointer\nreturned by gmtime(3), and localtime(3), and we always return a valid\npointer instead of NULL.\n\nDrop the memcpy(3) by using gmtime_s, and localtime_s as backend on\nWindows, and make sure we will return NULL in case of failure.\n\nCc: Johannes Sixt \nCc: Johannes Schindelin \nSigned-off-by: Doan Tran Cong Danh \n---\n compat/mingw.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex fe609239dd..7b21f4eee5 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1,6 +1,7 @@\n #include \"../git-compat-util.h\"\n #include \"win32.h\"\n #include \n+#include \n #include \n #include \"../strbuf.h\"\n #include \"../run-command.h\"\n@@ -986,16 +987,16 @@ int pipe(int filedes[2])\n \n struct tm *gmtime_r(const time_t *timep, struct tm *result)\n {\n-\t/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n-\tmemcpy(result, gmtime(timep), sizeof(struct tm));\n-\treturn result;\n+\tif (gmtime_s(result, timep) == 0)\n+\t\treturn result;\n+\treturn NULL;\n }\n \n struct tm *localtime_r(const time_t *timep, struct tm *result)\n {\n-\t/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n-\tmemcpy(result, localtime(timep), sizeof(struct tm));\n-\treturn result;\n+\tif (localtime_s(result, timep) == 0)\n+\t\treturn result;\n+\treturn NULL;\n }\n \n char *mingw_getcwd(char *pointer, int len)\n-- \n2.24.0.158.gd77a74f4dd.dirty\n\n"},{"id":"387175","messageId":"20191127162930.GC30581@sigill.intra.peff.net","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"Re: [PATCH 0/5] drop non-reentrant time usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-27T16:29:30Z","receivedAt":"2019-11-27T16:29:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 27, 2019 at 10:13:16PM +0700, Doan Tran Cong Danh wrote:\n\n> gmtime/localtime is considered unsafe in multithread environment.\n> \n> git was started as single-thread application, but we have some\n> multi-thread code, right now.\n> \n> replace all usage of gmtime/localtime by their respective reentrant ones.\n\nI think this is a good change.\n\nA minor point, but I think it may be simpler if the first four were just\na single patch. There's no rationale given at all in the 3rd and 4th\nones. Which is because you already explained it in patch 1, but that\nwon't help somebody who digs up the commit via \"git blame\".\n\nSo I think they either ought to be one patch, or they should repeat the\nrationale (I'd probably go with the first, but I could live with the\nsecond).\n\n> On Windows, we may be taxed, since gmtime_r and localtime_r is our compat\n> functions, because we memcpy from returned data of gmtime/localtime.\n> \n> To address that, I made patch #5 and included it together with this series.\n> I'm not sure how much portable it is.\n> It seems to run fine with my Windows 7 x86 and Windows 10 x64 VM, (inside\n> git-sdk for Windows, if it matters).\n> I'm Cc-ing j6t and Dscho on that patch to get their opinions.\n\nThat makes sense from my reading of gmtime_s(), but there may be\nsomething more subtle going on with compatibility, so I'll leave that to\nthe experts.\n\n-Peff\n"},{"id":"387210","messageId":"nycvar.QRO.7.76.6.1911272034550.31080@tvgsbejvaqbjf.bet","threadId":"52359","inReplyTo":"2c39f9a04f98fc3d365c80ce57e8edce305846fb.1574867409.git.congdanhqx@gmail.com","subject":"Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-27T19:35:41Z","receivedAt":"2019-11-27T19:36:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Danh,\n\nOn Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:\n\n> Since Windows doesn't provide gmtime_r(3) and localtime_r(3),\n> we're providing a compat version by using non-reentrant gmtime(3) and\n> localtime(3) as backend. Then, we copy the returned data into the\n> buffer.\n>\n> By doing that, in case of failure, we will dereference a NULL pointer\n> returned by gmtime(3), and localtime(3), and we always return a valid\n> pointer instead of NULL.\n>\n> Drop the memcpy(3) by using gmtime_s, and localtime_s as backend on\n\ns/and localtime_s/and use localtime_s()/\n\nOtherwise, this looks good to me, thank you!\n\nCiao,\nDscho\n\n> Windows, and make sure we will return NULL in case of failure.\n>\n> Cc: Johannes Sixt \n> Cc: Johannes Schindelin \n> Signed-off-by: Doan Tran Cong Danh \n> ---\n> compat/mingw.c | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index fe609239dd..7b21f4eee5 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1,6 +1,7 @@\n> #include \"../git-compat-util.h\"\n> #include \"win32.h\"\n> #include \n> +#include \n> #include \n> #include \"../strbuf.h\"\n> #include \"../run-command.h\"\n> @@ -986,16 +987,16 @@ int pipe(int filedes[2])\n>\n> struct tm *gmtime_r(const time_t *timep, struct tm *result)\n> {\n> -\t/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n> -\tmemcpy(result, gmtime(timep), sizeof(struct tm));\n> -\treturn result;\n> +\tif (gmtime_s(result, timep) == 0)\n> +\t\treturn result;\n> +\treturn NULL;\n> }\n>\n> struct tm *localtime_r(const time_t *timep, struct tm *result)\n> {\n> -\t/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n> -\tmemcpy(result, localtime(timep), sizeof(struct tm));\n> -\treturn result;\n> +\tif (localtime_s(result, timep) == 0)\n> +\t\treturn result;\n> +\treturn NULL;\n> }\n>\n> char *mingw_getcwd(char *pointer, int len)\n> --\n> 2.24.0.158.gd77a74f4dd.dirty\n>\n>\n"},{"id":"387211","messageId":"nycvar.QRO.7.76.6.1911272038430.31080@tvgsbejvaqbjf.bet","threadId":"52359","inReplyTo":"2c39f9a04f98fc3d365c80ce57e8edce305846fb.1574867409.git.congdanhqx@gmail.com","subject":"Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-27T19:39:27Z","receivedAt":"2019-11-27T19:39:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Danh,\n\nOn Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index fe609239dd..7b21f4eee5 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1,6 +1,7 @@\n> #include \"../git-compat-util.h\"\n> #include \"win32.h\"\n> #include \n> +#include \n\nI actually overlooked this. Please drop this hunk, it should not be needed\n(and might break things in the MSVC build).\n\nThanks,\nDscho\n\n> #include \n> #include \"../strbuf.h\"\n> #include \"../run-command.h\"\n> @@ -986,16 +987,16 @@ int pipe(int filedes[2])\n>\n> struct tm *gmtime_r(const time_t *timep, struct tm *result)\n> {\n> -\t/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n> -\tmemcpy(result, gmtime(timep), sizeof(struct tm));\n> -\treturn result;\n> +\tif (gmtime_s(result, timep) == 0)\n> +\t\treturn result;\n> +\treturn NULL;\n> }\n>\n> struct tm *localtime_r(const time_t *timep, struct tm *result)\n> {\n> -\t/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n> -\tmemcpy(result, localtime(timep), sizeof(struct tm));\n> -\treturn result;\n> +\tif (localtime_s(result, timep) == 0)\n> +\t\treturn result;\n> +\treturn NULL;\n> }\n>\n> char *mingw_getcwd(char *pointer, int len)\n> --\n> 2.24.0.158.gd77a74f4dd.dirty\n>\n>\n"},{"id":"387277","messageId":"20191128120533.GB9067@danh.dev","threadId":"52359","inReplyTo":"nycvar.QRO.7.76.6.1911272038430.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 5/5] mingw: use {gm,local}time_s as backend for {gm,local}time_r","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:05:33Z","receivedAt":"2019-11-28T12:05:37Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2019-11-27 20:39:27+0100, Johannes Schindelin wrote:\n> Hi Danh,\n> \n> On Wed, 27 Nov 2019, Doan Tran Cong Danh wrote:\n> \n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index fe609239dd..7b21f4eee5 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -1,6 +1,7 @@\n> > #include \"../git-compat-util.h\"\n> > #include \"win32.h\"\n> > #include \n> > +#include \n> \n> I actually overlooked this. Please drop this hunk, it should not be needed\n> (and might break things in the MSVC build).\n\nOops, originally, I intended to reset errno after {gm,local}time_s\nFound out it's unneccessary later but I forgot to drop this hunk.\nI'll fix it.\n\n-- \nDanh\n"},{"id":"387278","messageId":"20191128121613.GC9067@danh.dev","threadId":"52359","inReplyTo":"20191127162930.GC30581@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] drop non-reentrant time usage","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:16:13Z","receivedAt":"2019-11-28T12:16:18Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2019-11-27 11:29:30-0500, Jeff King wrote:\n> On Wed, Nov 27, 2019 at 10:13:16PM +0700, Doan Tran Cong Danh wrote:\n> \n> > gmtime/localtime is considered unsafe in multithread environment.\n> > \n> > git was started as single-thread application, but we have some\n> > multi-thread code, right now.\n> > \n> > replace all usage of gmtime/localtime by their respective reentrant ones.\n> \n> I think this is a good change.\n> \n> A minor point, but I think it may be simpler if the first four were just\n> a single patch. There's no rationale given at all in the 3rd and 4th\n> ones. Which is because you already explained it in patch 1, but that\n> won't help somebody who digs up the commit via \"git blame\".\n> \n> So I think they either ought to be one patch, or they should repeat the\n> rationale (I'd probably go with the first, but I could live with the\n> second).\n\nI'll merge first three into one since both of them are in date.c,\nthe 4th one changed another file.\n\n-- \nDanh\n"},{"id":"387279","messageId":"cover.1574943677.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574867409.git.congdanhqx@gmail.com","subject":"[PATCH v2 0/3] Phase out non-reentrant time functions","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:25:02Z","receivedAt":"2019-11-28T12:25:28Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"gmtime/localtime is considered unsafe in multithread environment.\n\ngit was started as single-thread application, but we have some\nmulti-thread code, right now.\n\nreplace all usage of gmtime/localtime by their respective reentrant ones.\n\nOn most POSIX systems, we won't notice any differences in performance,\nsince they implemented gmtime and gmtime_r mostly the same.\n\nOn Windows, we may be taxed, since gmtime_r and localtime_r is our compat\nfunctions, because we memcpy from returned data of gmtime/localtime.\n\nTo address that, I made patch #3 and included it together with this series.\nI'm not sure how much portable it is.\n\n\nDoan Tran Cong Danh (3):\n date.c: switch to reentrant {gm,local}time_r\n archive-zip.c: switch to reentrant localtime_r\n mingw: use {gm,local}time_s as backend for {gm,local}time_r\n\n archive-zip.c | 10 +++++-----\n compat/mingw.c | 12 ++++++------\n date.c | 18 ++++++++++--------\n 3 files changed, 21 insertions(+), 19 deletions(-)\n\nRange-diff against v1:\n1: 1e7e08b3c7 < -: ---------- date.c::datestamp: switch to reentrant localtime_r\n2: 130eba77db ! 1: 71de738864 date.c::time_to_tm_local: use reentrant localtime_r(3)\n @@ Metadata\n Author: Doan Tran Cong Danh \n \n ## Commit message ##\n - date.c::time_to_tm_local: use reentrant localtime_r(3)\n + date.c: switch to reentrant {gm,local}time_r\n \n - Phase out `localtime(3)' in favour of reentrant `localtime_r(3)'\n + Originally, git was intended to be single-thread executable.\n + `gmtime(3)' and `localtime(3)' can be used in such codebase\n + for cleaner code.\n +\n + Overtime, we're employing multithread in our code base.\n +\n + Let's phase out `gmtime(3)' and `localtime(3)' in favour of\n + `gmtime_r(3)' and `localtime_r(3)'.\n \n Signed-off-by: Doan Tran Cong Danh \n \n ## date.c ##\n -@@ date.c: static struct tm *time_to_tm(timestamp_t time, int tz)\n - \treturn gmtime(&t);\n +@@ date.c: static time_t gm_time_t(timestamp_t time, int tz)\n + * thing, which means that tz -0100 is passed in as the integer -100,\n + * even though it means \"sixty minutes off\"\n + */\n +-static struct tm *time_to_tm(timestamp_t time, int tz)\n ++static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)\n + {\n + \ttime_t t = gm_time_t(time, tz);\n +-\treturn gmtime(&t);\n ++\treturn gmtime_r(&t, tm);\n }\n \n -static struct tm *time_to_tm_local(timestamp_t time)\n @@ date.c: const char *show_date(timestamp_t time, int tz, const struct date_mode *\n -\t\ttm = time_to_tm_local(time);\n +\t\ttm = time_to_tm_local(time, &tmbuf);\n \telse\n - \t\ttm = time_to_tm(time, tz);\n +-\t\ttm = time_to_tm(time, tz);\n ++\t\ttm = time_to_tm(time, tz, &tmbuf);\n \tif (!tm) {\n +-\t\ttm = time_to_tm(0, 0);\n ++\t\ttm = time_to_tm(0, 0, &tmbuf);\n + \t\ttz = 0;\n + \t}\n + \n +@@ date.c: void datestamp(struct strbuf *out)\n + {\n + \ttime_t now;\n + \tint offset;\n ++\tstruct tm tm = { 0 };\n + \n + \ttime(&now);\n + \n +-\toffset = tm_to_time_t(localtime(&now)) - now;\n ++\toffset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n + \toffset /= 60;\n + \n + \tdate_string(now, offset, out);\n3: ce7fe7bcf3 < -: ---------- date.c::time_to_tm: use reentrant gmtime_r(3)\n4: f6fd89dfe1 ! 2: 77798f1773 archive-zip: use reentrant localtime_r(3)\n @@ Metadata\n Author: Doan Tran Cong Danh \n \n ## Commit message ##\n - archive-zip: use reentrant localtime_r(3)\n + archive-zip.c: switch to reentrant localtime_r\n +\n + Originally, git was intended to be single-thread executable.\n + `localtime(3)' can be used in such codebase for cleaner code.\n +\n + Overtime, we're employing multithread in our code base.\n +\n + Let's phase out `gmtime(3)' in favour of `localtime_r(3)'.\n +\n + Signed-off-by: Doan Tran Cong Danh \n \n ## archive-zip.c ##\n @@ archive-zip.c: static void write_zip_trailer(const struct object_id *oid)\n5: 2c39f9a04f ! 3: 33a67eb377 mingw: use {gm,local}time_s as backend for {gm,local}time_r\n @@ Commit message\n Signed-off-by: Doan Tran Cong Danh \n \n ## compat/mingw.c ##\n -@@\n - #include \"../git-compat-util.h\"\n - #include \"win32.h\"\n - #include \n -+#include \n - #include \n - #include \"../strbuf.h\"\n - #include \"../run-command.h\"\n @@ compat/mingw.c: int pipe(int filedes[2])\n \n struct tm *gmtime_r(const time_t *timep, struct tm *result)\n-- \n2.24.0.615.g37f5bfbdea\n\n"},{"id":"387280","messageId":"71de738864d273c925ed8a9a93fe1a85fa8b81b6.1574943677.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574943677.git.congdanhqx@gmail.com","subject":"[PATCH v2 1/3] date.c: switch to reentrant {gm,local}time_r","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:25:03Z","receivedAt":"2019-11-28T12:25:32Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Originally, git was intended to be single-thread executable.\n`gmtime(3)' and `localtime(3)' can be used in such codebase\nfor cleaner code.\n\nOvertime, we're employing multithread in our code base.\n\nLet's phase out `gmtime(3)' and `localtime(3)' in favour of\n`gmtime_r(3)' and `localtime_r(3)'.\n\nSigned-off-by: Doan Tran Cong Danh \n---\n date.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 041db7db4e..b0d9a8421d 100644\n--- a/date.c\n+++ b/date.c\n@@ -64,16 +64,16 @@ static time_t gm_time_t(timestamp_t time, int tz)\n * thing, which means that tz -0100 is passed in as the integer -100,\n * even though it means \"sixty minutes off\"\n */\n-static struct tm *time_to_tm(timestamp_t time, int tz)\n+static struct tm *time_to_tm(timestamp_t time, int tz, struct tm *tm)\n {\n \ttime_t t = gm_time_t(time, tz);\n-\treturn gmtime(&t);\n+\treturn gmtime_r(&t, tm);\n }\n \n-static struct tm *time_to_tm_local(timestamp_t time)\n+static struct tm *time_to_tm_local(timestamp_t time, struct tm *tm)\n {\n \ttime_t t = time;\n-\treturn localtime(&t);\n+\treturn localtime_r(&t, tm);\n }\n \n /*\n@@ -283,6 +283,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n {\n \tstruct tm *tm;\n+\tstruct tm tmbuf = { 0 };\n \tstruct tm human_tm = { 0 };\n \tint human_tz = -1;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n@@ -318,11 +319,11 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t}\n \n \tif (mode->local)\n-\t\ttm = time_to_tm_local(time);\n+\t\ttm = time_to_tm_local(time, &tmbuf);\n \telse\n-\t\ttm = time_to_tm(time, tz);\n+\t\ttm = time_to_tm(time, tz, &tmbuf);\n \tif (!tm) {\n-\t\ttm = time_to_tm(0, 0);\n+\t\ttm = time_to_tm(0, 0, &tmbuf);\n \t\ttz = 0;\n \t}\n \n@@ -959,10 +960,11 @@ void datestamp(struct strbuf *out)\n {\n \ttime_t now;\n \tint offset;\n+\tstruct tm tm = { 0 };\n \n \ttime(&now);\n \n-\toffset = tm_to_time_t(localtime(&now)) - now;\n+\toffset = tm_to_time_t(localtime_r(&now, &tm)) - now;\n \toffset /= 60;\n \n \tdate_string(now, offset, out);\n-- \n2.24.0.615.g37f5bfbdea\n\n"},{"id":"387281","messageId":"77798f17730b3c3ad739a9f4a73072e5bb6ac5d9.1574943677.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574943677.git.congdanhqx@gmail.com","subject":"[PATCH v2 2/3] archive-zip.c: switch to reentrant localtime_r","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:25:04Z","receivedAt":"2019-11-28T12:25:46Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Originally, git was intended to be single-thread executable.\n`localtime(3)' can be used in such codebase for cleaner code.\n\nOvertime, we're employing multithread in our code base.\n\nLet's phase out `gmtime(3)' in favour of `localtime_r(3)'.\n\nSigned-off-by: Doan Tran Cong Danh \n---\n archive-zip.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 4d66b5be6e..8bbd3e5884 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -603,18 +603,18 @@ static void write_zip_trailer(const struct object_id *oid)\n static void dos_time(timestamp_t *timestamp, int *dos_date, int *dos_time)\n {\n \ttime_t time;\n-\tstruct tm *t;\n+\tstruct tm tm;\n \n \tif (date_overflows(*timestamp))\n \t\tdie(_(\"timestamp too large for this system: %\"PRItime),\n \t\t *timestamp);\n \ttime = (time_t)*timestamp;\n-\tt = localtime(&time);\n+\tlocaltime_r(&time, &tm);\n \t*timestamp = time;\n \n-\t*dos_date = t->tm_mday + (t->tm_mon + 1) * 32 +\n-\t (t->tm_year + 1900 - 1980) * 512;\n-\t*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;\n+\t*dos_date = tm.tm_mday + (tm.tm_mon + 1) * 32 +\n+\t (tm.tm_year + 1900 - 1980) * 512;\n+\t*dos_time = tm.tm_sec / 2 + tm.tm_min * 32 + tm.tm_hour * 2048;\n }\n \n static int archive_zip_config(const char *var, const char *value, void *data)\n-- \n2.24.0.615.g37f5bfbdea\n\n"},{"id":"387282","messageId":"33a67eb37702172a6ff29215a840d2cf925daab8.1574943677.git.congdanhqx@gmail.com","threadId":"52359","inReplyTo":"cover.1574943677.git.congdanhqx@gmail.com","subject":"[PATCH v2 3/3] mingw: use {gm,local}time_s as backend for {gm,local}time_r","fromName":"Doan Tran Cong Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-28T12:25:05Z","receivedAt":"2019-11-28T12:25:53Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Since Windows doesn't provide gmtime_r(3) and localtime_r(3),\nwe're providing a compat version by using non-reentrant gmtime(3) and\nlocaltime(3) as backend. Then, we copy the returned data into the\nbuffer.\n\nBy doing that, in case of failure, we will dereference a NULL pointer\nreturned by gmtime(3), and localtime(3), and we always return a valid\npointer instead of NULL.\n\nDrop the memcpy(3) by using gmtime_s, and localtime_s as backend on\nWindows, and make sure we will return NULL in case of failure.\n\nCc: Johannes Sixt \nCc: Johannes Schindelin \nSigned-off-by: Doan Tran Cong Danh \n---\n compat/mingw.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex fe609239dd..75695a24a3 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -986,16 +986,16 @@ int pipe(int filedes[2])\n \n struct tm *gmtime_r(const time_t *timep, struct tm *result)\n {\n-\t/* gmtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n-\tmemcpy(result, gmtime(timep), sizeof(struct tm));\n-\treturn result;\n+\tif (gmtime_s(result, timep) == 0)\n+\t\treturn result;\n+\treturn NULL;\n }\n \n struct tm *localtime_r(const time_t *timep, struct tm *result)\n {\n-\t/* localtime() in MSVCRT.DLL is thread-safe, but not reentrant */\n-\tmemcpy(result, localtime(timep), sizeof(struct tm));\n-\treturn result;\n+\tif (localtime_s(result, timep) == 0)\n+\t\treturn result;\n+\treturn NULL;\n }\n \n char *mingw_getcwd(char *pointer, int len)\n-- \n2.24.0.615.g37f5bfbdea\n\n"}]}