{"thread":{"id":"54734","subject":"[PATCH] builtin/bugreport.c: use thread-safe localtime_r()","startedAt":"2020-11-30T23:07:53Z","lastAt":"2020-12-06T14:57:28Z","messageCount":14,"participants":["Taylor Blau","Eric Sunshine","Jeff King","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"411017","messageId":"27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com","threadId":"54734","inReplyTo":null,"subject":"[PATCH] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-30T23:06:44Z","receivedAt":"2020-11-30T23:07:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To generate its filename, the 'git bugreport' builtin asks the system\nfor the current time with 'localtime()'. Since this uses a shared\nbuffer, it is not thread-safe.\n\nEven though 'git bugreport' is not multi-threaded, using localtime() can\ntrigger some static analysis tools to complain, and a quick\n\n    $ git grep -oh 'localtime\\(_.\\)\\?' -- **/*.c | sort | uniq -c\n\nshows that the only usage of the thread-unsafe 'localtime' is in a piece\nof documentation.\n\nSo, convert this instance to use the thread-safe version for\nconsistency, and to appease some analysis tools.\n---\nSome folks at GitHub sent me the output of a static analysis tool run\nagainst our private fork, and this usage of 'localtime()' showed up.\n\nThis is purely academic, since this clearly isn't a thread-unsafe usage\nof that function, but it should appease any other static analysis tools\nthat folks might run.\n\n builtin/bugreport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 3ad4b9b62e..ad3cc9c02f 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -125,6 +125,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstruct strbuf report_path = STRBUF_INIT;\n \tint report = -1;\n \ttime_t now = time(NULL);\n+\tstruct tm tm;\n \tchar *option_output = NULL;\n \tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n \tconst char *user_relative_path = NULL;\n@@ -147,7 +148,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_complete(&report_path, '/');\n\n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);\n+\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n \tstrbuf_addstr(&report_path, \".txt\");\n\n \tswitch (safe_create_leading_directories(report_path.buf)) {\n--\n2.29.2.533.g07db1f5344\n"},{"id":"411021","messageId":"73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me@ttaylorr.com","threadId":"54734","inReplyTo":"27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-01T00:30:06Z","receivedAt":"2020-12-01T00:30:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To generate its filename, the 'git bugreport' builtin asks the system\nfor the current time with 'localtime()'. Since this uses a shared\nbuffer, it is not thread-safe.\n\nEven though 'git bugreport' is not multi-threaded, using localtime() can\ntrigger some static analysis tools to complain, and a quick\n\n    $ git grep -oh 'localtime\\(_.\\)\\?' -- **/*.c | sort | uniq -c\n\nshows that the only usage of the thread-unsafe 'localtime' is in a piece\nof documentation.\n\nSo, convert this instance to use the thread-safe version for\nconsistency, and to appease some analysis tools.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nHow embarrassing: I forgot my sign-off on the previous message. The\ncontents in this version are unchanged, but this one includes my\nsign-off.\n\n builtin/bugreport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 3ad4b9b62e..ad3cc9c02f 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -125,6 +125,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstruct strbuf report_path = STRBUF_INIT;\n \tint report = -1;\n \ttime_t now = time(NULL);\n+\tstruct tm tm;\n \tchar *option_output = NULL;\n \tchar *option_suffix = \"%Y-%m-%d-%H%M\";\n \tconst char *user_relative_path = NULL;\n@@ -147,7 +148,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_complete(&report_path, '/');\n\n \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);\n+\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n \tstrbuf_addstr(&report_path, \".txt\");\n\n \tswitch (safe_create_leading_directories(report_path.buf)) {\n--\n2.29.2.533.g07db1f5344\n"},{"id":"411022","messageId":"CAPig+cRx03potus-ea-4J8mCuG3vVeQBJ8NcEh_Hs2yJqaoXcw@mail.gmail.com","threadId":"54734","inReplyTo":"27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-01T00:31:49Z","receivedAt":"2020-12-01T00:32:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 30, 2020 at 6:10 PM Taylor Blau <me@ttaylorr.com> wrote:\n> To generate its filename, the 'git bugreport' builtin asks the system\n> for the current time with 'localtime()'. Since this uses a shared\n> buffer, it is not thread-safe.\n>\n> Even though 'git bugreport' is not multi-threaded, using localtime() can\n> trigger some static analysis tools to complain, and a quick\n>\n>     $ git grep -oh 'localtime\\(_.\\)\\?' -- **/*.c | sort | uniq -c\n>\n> shows that the only usage of the thread-unsafe 'localtime' is in a piece\n> of documentation.\n>\n> So, convert this instance to use the thread-safe version for\n> consistency, and to appease some analysis tools.\n> ---\n\nMissing sign-off.\n\n> This is purely academic, since this clearly isn't a thread-unsafe usage\n> of that function, but it should appease any other static analysis tools\n> that folks might run.\n\nIt's not only multi-threaded cases for which it could be a problem,\nbut also cases in which the caller holds onto the pointer to the\nreturned shared buffer assuming it will remain valid until use. If the\ncaller invokes some other code which itself calls localtime(), then\nthe buffer might be overwritten before the original caller uses the\nvalue. But, you're right that in this particular case it's academic\nsince strbuf_addftime() doesn't do anything which should clobber the\nshared buffer.\n\nThe patch itself looks fine.\n"},{"id":"411029","messageId":"X8WqFynk23yWT6E3@coredump.intra.peff.net","threadId":"54734","inReplyTo":"73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T02:27:35Z","receivedAt":"2020-12-01T02:28:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 30, 2020 at 07:30:06PM -0500, Taylor Blau wrote:\n\n> @@ -147,7 +148,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_complete(&report_path, '/');\n> \n>  \tstrbuf_addstr(&report_path, \"git-bugreport-\");\n> -\tstrbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);\n> +\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n>  \tstrbuf_addstr(&report_path, \".txt\");\n\nI briefly wondered if we'd want a strbuf_addftime() variant that just\ntakes a time_t. But the choice of localtime vs gmtime makes this\nawkward, not to mention the gymnastics we do in show_date() to get\nthings into the author's zone. So this looks good to me.\n\nWe might also want to do this on top:\n\n-- >8 --\nSubject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned\n\nThe traditional gmtime(), localtime(), ctime(), and asctime() functions\nreturn pointers to shared storage. This means they're not thread-safe,\nand they also run the risk of somebody holding onto the result across\nmultiple calls (where each call invalidates the previous result).\n\nAll callers should be using gmtime_r() or localtime_r() instead.\n\nThe ctime_r() and asctime_r() functions are OK in that respect, but have\nno check that the buffer we pass in is long enough (the manpage says it\n\"should have room for at least 26 bytes\"). Since this is such an\neasy-to-get-wrong interface, and since we have the much safer stftime()\nas well as its more conveinent strbuf_addftime() wrapper, let's likewise\nban both of those.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTBH, ctime() and its variants are so awful that I doubt anybody would\ntry to use them, but it doesn't hurt to err on the side of caution.\n\n banned.h | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 60a18d4403..7ab4f2e492 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -29,4 +29,17 @@\n #define vsprintf(buf,fmt,arg) BANNED(vsprintf)\n #endif\n \n+#undef gmtime\n+#define gmtime(t) BANNED(gmtime)\n+#undef localtime\n+#define localtime(t) BANNED(localtime)\n+#undef ctime\n+#define ctime(t) BANNED(ctime)\n+#undef ctime_r\n+#define ctime_r(t, buf) BANNED(ctime_r)\n+#undef asctime\n+#define asctime(t) BANNED(asctime)\n+#undef asctime_r\n+#define asctime_r(t, buf) BANNED(asctime_r)\n+\n #endif /* BANNED_H */\n-- \n2.29.2.853.g04e16501f9\n\n"},{"id":"411031","messageId":"CAPig+cT=gMEuKkbJefT9yxWWB5VC1fj6T+ofjn_saEEeEeU_MA@mail.gmail.com","threadId":"54734","inReplyTo":"X8WqFynk23yWT6E3@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-01T03:15:37Z","receivedAt":"2020-12-01T03:16:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 30, 2020 at 9:30 PM Jeff King <peff@peff.net> wrote:\n> We might also want to do this on top:\n>\n> -- >8 --\n> Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned\n>\n> The traditional gmtime(), localtime(), ctime(), and asctime() functions\n> return pointers to shared storage. This means they're not thread-safe,\n> and they also run the risk of somebody holding onto the result across\n> multiple calls (where each call invalidates the previous result).\n>\n> All callers should be using gmtime_r() or localtime_r() instead.\n>\n> The ctime_r() and asctime_r() functions are OK in that respect, but have\n> no check that the buffer we pass in is long enough (the manpage says it\n> \"should have room for at least 26 bytes\"). Since this is such an\n> easy-to-get-wrong interface, and since we have the much safer stftime()\n> as well as its more conveinent strbuf_addftime() wrapper, let's likewise\n> ban both of those.\n\ns/conveinent/convenient/\n\nI forgot all about banned.h. This patch does seem worthwhile to take.\n"},{"id":"411075","messageId":"xmqqlfehqt4n.fsf@gitster.c.googlers.com","threadId":"54734","inReplyTo":"X8WqFynk23yWT6E3@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T18:27:20Z","receivedAt":"2020-12-01T18:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We might also want to do this on top:\n>\n> -- >8 --\n> Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned\n\nI see the patch does more than what subject describes.  \n\nI am not opposed to banning ctime_r() and asctime_r(), but I do not\nwant to see our future readers wonder why they are banned by the\ncommit whose title clearly states that we refuse non-reentrant ones\nin our codebase.\n\nThanks.\n\n> The traditional gmtime(), localtime(), ctime(), and asctime() functions\n> return pointers to shared storage. This means they're not thread-safe,\n> and they also run the risk of somebody holding onto the result across\n> multiple calls (where each call invalidates the previous result).\n>\n> All callers should be using gmtime_r() or localtime_r() instead.\n>\n> The ctime_r() and asctime_r() functions are OK in that respect, but have\n> no check that the buffer we pass in is long enough (the manpage says it\n> \"should have room for at least 26 bytes\"). Since this is such an\n> easy-to-get-wrong interface, and since we have the much safer stftime()\n> as well as its more conveinent strbuf_addftime() wrapper, let's likewise\n> ban both of those.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> TBH, ctime() and its variants are so awful that I doubt anybody would\n> try to use them, but it doesn't hurt to err on the side of caution.\n>\n>  banned.h | 13 +++++++++++++\n>  1 file changed, 13 insertions(+)\n>\n> diff --git a/banned.h b/banned.h\n> index 60a18d4403..7ab4f2e492 100644\n> --- a/banned.h\n> +++ b/banned.h\n> @@ -29,4 +29,17 @@\n>  #define vsprintf(buf,fmt,arg) BANNED(vsprintf)\n>  #endif\n>  \n> +#undef gmtime\n> +#define gmtime(t) BANNED(gmtime)\n> +#undef localtime\n> +#define localtime(t) BANNED(localtime)\n> +#undef ctime\n> +#define ctime(t) BANNED(ctime)\n> +#undef ctime_r\n> +#define ctime_r(t, buf) BANNED(ctime_r)\n> +#undef asctime\n> +#define asctime(t) BANNED(asctime)\n> +#undef asctime_r\n> +#define asctime_r(t, buf) BANNED(asctime_r)\n> +\n>  #endif /* BANNED_H */\n"},{"id":"411079","messageId":"X8aMt2LEiCLkdV9/@nand.local","threadId":"54734","inReplyTo":"xmqqlfehqt4n.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-01T18:34:31Z","receivedAt":"2020-12-01T18:35:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote:\n> I am not opposed to banning ctime_r() and asctime_r(), but I do not\n> want to see our future readers wonder why they are banned by the\n> commit whose title clearly states that we refuse non-reentrant ones\n> in our codebase.\n\nAgreed. Maybe splitting these into two (one to ban non-reentrant\nfunctions, and another to ban ctime_r() and asctime_r()) would help.\n\nThanks,\nTaylor\n"},{"id":"411104","messageId":"20201201211138.33850-1-gitster@pobox.com","threadId":"54734","inReplyTo":"X8aMt2LEiCLkdV9/@nand.local","subject":"[PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T21:11:37Z","receivedAt":"2020-12-01T21:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nThe traditional gmtime(), localtime(), ctime(), and asctime() functions\nreturn pointers to shared storage. This means they're not thread-safe,\nand they also run the risk of somebody holding onto the result across\nmultiple calls (where each call invalidates the previous result).\n\nAll callers should be using their reentrant counterparts.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n banned.h | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 60a18d4403..ed11300bb2 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -29,4 +29,13 @@\n #define vsprintf(buf,fmt,arg) BANNED(vsprintf)\n #endif\n \n+#undef gmtime\n+#define gmtime(t) BANNED(gmtime)\n+#undef localtime\n+#define localtime(t) BANNED(localtime)\n+#undef ctime\n+#define ctime(t) BANNED(ctime)\n+#undef asctime\n+#define asctime(t) BANNED(asctime)\n+\n #endif /* BANNED_H */\n-- \n2.29.2-561-g49e167ef76\n\n"},{"id":"411105","messageId":"20201201211138.33850-2-gitster@pobox.com","threadId":"54734","inReplyTo":"20201201211138.33850-1-gitster@pobox.com","subject":"[PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T21:11:38Z","receivedAt":"2020-12-01T21:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nThe ctime_r() and asctime_r() functions are reentrant, but have\nno check that the buffer we pass in is long enough (the manpage says it\n\"should have room for at least 26 bytes\"). Since this is such an\neasy-to-get-wrong interface, and since we have the much safer stftime()\nas well as its more conveinent strbuf_addftime() wrapper, let's ban both\nof those.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n banned.h | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex ed11300bb2..7ab4f2e492 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -35,7 +35,11 @@\n #define localtime(t) BANNED(localtime)\n #undef ctime\n #define ctime(t) BANNED(ctime)\n+#undef ctime_r\n+#define ctime_r(t, buf) BANNED(ctime_r)\n #undef asctime\n #define asctime(t) BANNED(asctime)\n+#undef asctime_r\n+#define asctime_r(t, buf) BANNED(asctime_r)\n \n #endif /* BANNED_H */\n-- \n2.29.2-561-g49e167ef76\n\n"},{"id":"411106","messageId":"CAPig+cROG5+khWvBWbWgVhNuDyWkCQYBXwte5VeazuCCXMAA_g@mail.gmail.com","threadId":"54734","inReplyTo":"20201201211138.33850-2-gitster@pobox.com","subject":"Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-01T21:16:11Z","receivedAt":"2020-12-01T21:17:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n> The ctime_r() and asctime_r() functions are reentrant, but have\n> no check that the buffer we pass in is long enough (the manpage says it\n> \"should have room for at least 26 bytes\"). Since this is such an\n> easy-to-get-wrong interface, and since we have the much safer stftime()\n> as well as its more conveinent strbuf_addftime() wrapper, let's ban both\n> of those.\n\nThis still needs a s/conveinent/convenient/ mentioned earlier[1].\n\n[1]: https://lore.kernel.org/git/CAPig+cT=gMEuKkbJefT9yxWWB5VC1fj6T+ofjn_saEEeEeU_MA@mail.gmail.com/\n"},{"id":"411108","messageId":"xmqqzh2xmb7o.fsf@gitster.c.googlers.com","threadId":"54734","inReplyTo":"CAPig+cROG5+khWvBWbWgVhNuDyWkCQYBXwte5VeazuCCXMAA_g@mail.gmail.com","subject":"Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T22:07:55Z","receivedAt":"2020-12-01T22:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> The ctime_r() and asctime_r() functions are reentrant, but have\n>> no check that the buffer we pass in is long enough (the manpage says it\n>> \"should have room for at least 26 bytes\"). Since this is such an\n>> easy-to-get-wrong interface, and since we have the much safer stftime()\n>> as well as its more conveinent strbuf_addftime() wrapper, let's ban both\n>> of those.\n>\n> This still needs a s/conveinent/convenient/ mentioned earlier[1].\n\nAH, thanks, fixed.\n"},{"id":"411109","messageId":"X8bCJ/ZukppAsoTY@nand.local","threadId":"54734","inReplyTo":"xmqqzh2xmb7o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2020-12-01T22:22:50Z","receivedAt":"2020-12-01T22:23:40Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Tue, Dec 01, 2020 at 02:07:55PM -0800, Junio C Hamano wrote:\n> > This still needs a s/conveinent/convenient/ mentioned earlier[1].\n>\n> AH, thanks, fixed.\n\nThat was my only nit, but I'm certainly quite happy to see these get\npicked up.\n\nHere's my:\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nif you need it (which I doubt, but there it is anyway).\n\n\nThanks,\nTaylor\n"},{"id":"411132","messageId":"X8b0he9VVI5s1log@coredump.intra.peff.net","threadId":"54734","inReplyTo":"xmqqlfehqt4n.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-02T01:57:25Z","receivedAt":"2020-12-02T01:58:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We might also want to do this on top:\n> >\n> > -- >8 --\n> > Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned\n> \n> I see the patch does more than what subject describes.  \n> \n> I am not opposed to banning ctime_r() and asctime_r(), but I do not\n> want to see our future readers wonder why they are banned by the\n> commit whose title clearly states that we refuse non-reentrant ones\n> in our codebase.\n\nWell, not more than the overall commit message describes. :)\n\nBut yeah, the split in what you re-sent is just fine with me. Thanks for\nsaving a round-trip. I see you already fixed up the typo in the second\none pointed out by Eric, but I think there is another:\n\n  s/stftime/strftime/\n\n-Peff\n"},{"id":"411564","messageId":"20201206145642.GH8396@szeder.dev","threadId":"54734","inReplyTo":"20201201211138.33850-1-gitster@pobox.com","subject":"Re: [PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-12-06T14:56:42Z","receivedAt":"2020-12-06T14:57:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 01, 2020 at 01:11:37PM -0800, Junio C Hamano wrote:\n> From: Jeff King <peff@peff.net>\n> \n> The traditional gmtime(), localtime(), ctime(), and asctime() functions\n> return pointers to shared storage. This means they're not thread-safe,\n> and they also run the risk of somebody holding onto the result across\n> multiple calls (where each call invalidates the previous result).\n> \n> All callers should be using their reentrant counterparts.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  banned.h | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n> \n> diff --git a/banned.h b/banned.h\n> index 60a18d4403..ed11300bb2 100644\n> --- a/banned.h\n> +++ b/banned.h\n> @@ -29,4 +29,13 @@\n>  #define vsprintf(buf,fmt,arg) BANNED(vsprintf)\n>  #endif\n>  \n> +#undef gmtime\n> +#define gmtime(t) BANNED(gmtime)\n> +#undef localtime\n> +#define localtime(t) BANNED(localtime)\n> +#undef ctime\n> +#define ctime(t) BANNED(ctime)\n> +#undef asctime\n> +#define asctime(t) BANNED(asctime)\n> +\n>  #endif /* BANNED_H */\n\nThis patch should be queued on top of topic\n'tb/bugreport-no-localtime'.  Currently they are on parallel branches:\n\n  * 91aef03015 (refs/upstream/jk/banned) banned.h: mark ctime_r() and asctime_r() as banned\n  * 1fbfdf556f banned.h: mark non-reentrant gmtime, etc as banned\n  | * 4f6460df55 (refs/upstream/tb/bugreport-no-localtime) builtin/bugreport.c: use thread-safe localtime_r()\n  |/  \n  * 72ffeb997e Ninth batch\n\nand because of the not-yet-removed localtime() call in\n'builtin/bugreport.c' commits 1fbfdf556f and 91aef03015 can't be\nbuilt.\n\n"}]}