Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r()

[Jeff King <peff@peff.net>]

On Mon, Nov 30, 2020 at 07:30:06PM -0500, Taylor Blau wrote:

> @@ -147,7 +148,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	strbuf_complete(&report_path, '/');
> 
>  	strbuf_addstr(&report_path, "git-bugreport-");
> -	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
> +	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
>  	strbuf_addstr(&report_path, ".txt");

I briefly wondered if we'd want a strbuf_addftime() variant that just
takes a time_t. But the choice of localtime vs gmtime makes this
awkward, not to mention the gymnastics we do in show_date() to get
things into the author's zone. So this looks good to me.

We might also want to do this on top:

-- >8 --
Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned

The traditional gmtime(), localtime(), ctime(), and asctime() functions
return pointers to shared storage. This means they're not thread-safe,
and they also run the risk of somebody holding onto the result across
multiple calls (where each call invalidates the previous result).

All callers should be using gmtime_r() or localtime_r() instead.

The ctime_r() and asctime_r() functions are OK in that respect, but have
no check that the buffer we pass in is long enough (the manpage says it
"should have room for at least 26 bytes"). Since this is such an
easy-to-get-wrong interface, and since we have the much safer stftime()
as well as its more conveinent strbuf_addftime() wrapper, let's likewise
ban both of those.

Signed-off-by: Jeff King <peff@peff.net>
---
TBH, ctime() and its variants are so awful that I doubt anybody would
try to use them, but it doesn't hurt to err on the side of caution.

 banned.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/banned.h b/banned.h
index 60a18d4403..7ab4f2e492 100644
--- a/banned.h
+++ b/banned.h
@@ -29,4 +29,17 @@
 #define vsprintf(buf,fmt,arg) BANNED(vsprintf)
 #endif
 
+#undef gmtime
+#define gmtime(t) BANNED(gmtime)
+#undef localtime
+#define localtime(t) BANNED(localtime)
+#undef ctime
+#define ctime(t) BANNED(ctime)
+#undef ctime_r
+#define ctime_r(t, buf) BANNED(ctime_r)
+#undef asctime
+#define asctime(t) BANNED(asctime)
+#undef asctime_r
+#define asctime_r(t, buf) BANNED(asctime_r)
+
 #endif /* BANNED_H */
-- 
2.29.2.853.g04e16501f9