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

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

[Taylor Blau]

To generate its filename, the 'git bugreport' builtin asks the system
for the current time with 'localtime()'. Since this uses a shared
buffer, it is not thread-safe.

Even though 'git bugreport' is not multi-threaded, using localtime() can
trigger some static analysis tools to complain, and a quick

    $ git grep -oh 'localtime\(_.\)\?' -- **/*.c | sort | uniq -c

shows that the only usage of the thread-unsafe 'localtime' is in a piece
of documentation.

So, convert this instance to use the thread-safe version for
consistency, and to appease some analysis tools.
---
Some folks at GitHub sent me the output of a static analysis tool run
against our private fork, and this usage of 'localtime()' showed up.

This is purely academic, since this clearly isn't a thread-unsafe usage
of that function, but it should appease any other static analysis tools
that folks might run.

 builtin/bugreport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 3ad4b9b62e..ad3cc9c02f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -125,6 +125,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	struct strbuf report_path = STRBUF_INIT;
 	int report = -1;
 	time_t now = time(NULL);
+	struct tm tm;
 	char *option_output = NULL;
 	char *option_suffix = "%Y-%m-%d-%H%M";
 	const char *user_relative_path = NULL;
@@ -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");

 	switch (safe_create_leading_directories(report_path.buf)) {
--
2.29.2.533.g07db1f5344

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

[Taylor Blau]

To generate its filename, the 'git bugreport' builtin asks the system
for the current time with 'localtime()'. Since this uses a shared
buffer, it is not thread-safe.

Even though 'git bugreport' is not multi-threaded, using localtime() can
trigger some static analysis tools to complain, and a quick

    $ git grep -oh 'localtime\(_.\)\?' -- **/*.c | sort | uniq -c

shows that the only usage of the thread-unsafe 'localtime' is in a piece
of documentation.

So, convert this instance to use the thread-safe version for
consistency, and to appease some analysis tools.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
How embarrassing: I forgot my sign-off on the previous message. The
contents in this version are unchanged, but this one includes my
sign-off.

 builtin/bugreport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 3ad4b9b62e..ad3cc9c02f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -125,6 +125,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	struct strbuf report_path = STRBUF_INIT;
 	int report = -1;
 	time_t now = time(NULL);
+	struct tm tm;
 	char *option_output = NULL;
 	char *option_suffix = "%Y-%m-%d-%H%M";
 	const char *user_relative_path = NULL;
@@ -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");

 	switch (safe_create_leading_directories(report_path.buf)) {
--
2.29.2.533.g07db1f5344

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

[Eric Sunshine]

On Mon, Nov 30, 2020 at 6:10 PM Taylor Blau <me@ttaylorr.com> wrote:
> To generate its filename, the 'git bugreport' builtin asks the system
> for the current time with 'localtime()'. Since this uses a shared
> buffer, it is not thread-safe.
>
> Even though 'git bugreport' is not multi-threaded, using localtime() can
> trigger some static analysis tools to complain, and a quick
>
>     $ git grep -oh 'localtime\(_.\)\?' -- **/*.c | sort | uniq -c
>
> shows that the only usage of the thread-unsafe 'localtime' is in a piece
> of documentation.
>
> So, convert this instance to use the thread-safe version for
> consistency, and to appease some analysis tools.
> ---

Missing sign-off.

> This is purely academic, since this clearly isn't a thread-unsafe usage
> of that function, but it should appease any other static analysis tools
> that folks might run.

It's not only multi-threaded cases for which it could be a problem,
but also cases in which the caller holds onto the pointer to the
returned shared buffer assuming it will remain valid until use. If the
caller invokes some other code which itself calls localtime(), then
the buffer might be overwritten before the original caller uses the
value. But, you're right that in this particular case it's academic
since strbuf_addftime() doesn't do anything which should clobber the
shared buffer.

The patch itself looks fine.

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

[Jeff King]

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

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

[Eric Sunshine]

On Mon, Nov 30, 2020 at 9:30 PM Jeff King <peff@peff.net> wrote:
> 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.

s/conveinent/convenient/

I forgot all about banned.h. This patch does seem worthwhile to take.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We might also want to do this on top:
>
> -- >8 --
> Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned

I see the patch does more than what subject describes.  

I am not opposed to banning ctime_r() and asctime_r(), but I do not
want to see our future readers wonder why they are banned by the
commit whose title clearly states that we refuse non-reentrant ones
in our codebase.

Thanks.

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

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

[Taylor Blau]

On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote:
> I am not opposed to banning ctime_r() and asctime_r(), but I do not
> want to see our future readers wonder why they are banned by the
> commit whose title clearly states that we refuse non-reentrant ones
> in our codebase.

Agreed. Maybe splitting these into two (one to ban non-reentrant
functions, and another to ban ctime_r() and asctime_r()) would help.

Thanks,
Taylor

[PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned

[Junio C Hamano]

From: Jeff King <peff@peff.net>

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 their reentrant counterparts.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 banned.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/banned.h b/banned.h
index 60a18d4403..ed11300bb2 100644
--- a/banned.h
+++ b/banned.h
@@ -29,4 +29,13 @@
 #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 asctime
+#define asctime(t) BANNED(asctime)
+
 #endif /* BANNED_H */
-- 
2.29.2-561-g49e167ef76

[PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.

[Junio C Hamano]

From: Jeff King <peff@peff.net>

The ctime_r() and asctime_r() functions are reentrant, 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 ban both
of those.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 banned.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/banned.h b/banned.h
index ed11300bb2..7ab4f2e492 100644
--- a/banned.h
+++ b/banned.h
@@ -35,7 +35,11 @@
 #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-561-g49e167ef76

Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.

[Eric Sunshine]

On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <gitster@pobox.com> wrote:
> The ctime_r() and asctime_r() functions are reentrant, 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 ban both
> of those.

This still needs a s/conveinent/convenient/ mentioned earlier[1].

[1]: https://lore.kernel.org/git/CAPig+cT=gMEuKkbJefT9yxWWB5VC1fj6T+ofjn_saEEeEeU_MA@mail.gmail.com/

Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>> The ctime_r() and asctime_r() functions are reentrant, 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 ban both
>> of those.
>
> This still needs a s/conveinent/convenient/ mentioned earlier[1].

AH, thanks, fixed.

Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned.

[Taylor Blau]

On Tue, Dec 01, 2020 at 02:07:55PM -0800, Junio C Hamano wrote:
> > This still needs a s/conveinent/convenient/ mentioned earlier[1].
>
> AH, thanks, fixed.

That was my only nit, but I'm certainly quite happy to see these get
picked up.

Here's my:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

if you need it (which I doubt, but there it is anyway).


Thanks,
Taylor

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

[Jeff King]

On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We might also want to do this on top:
> >
> > -- >8 --
> > Subject: [PATCH] banned.h: mark non-reentrant gmtime, etc as banned
> 
> I see the patch does more than what subject describes.  
> 
> I am not opposed to banning ctime_r() and asctime_r(), but I do not
> want to see our future readers wonder why they are banned by the
> commit whose title clearly states that we refuse non-reentrant ones
> in our codebase.

Well, not more than the overall commit message describes. :)

But yeah, the split in what you re-sent is just fine with me. Thanks for
saving a round-trip. I see you already fixed up the typo in the second
one pointed out by Eric, but I think there is another:

  s/stftime/strftime/

-Peff

Re: [PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned

[SZEDER Gábor]

On Tue, Dec 01, 2020 at 01:11:37PM -0800, Junio C Hamano wrote:
> From: Jeff King <peff@peff.net>
> 
> 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 their reentrant counterparts.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  banned.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/banned.h b/banned.h
> index 60a18d4403..ed11300bb2 100644
> --- a/banned.h
> +++ b/banned.h
> @@ -29,4 +29,13 @@
>  #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 asctime
> +#define asctime(t) BANNED(asctime)
> +
>  #endif /* BANNED_H */

This patch should be queued on top of topic
'tb/bugreport-no-localtime'.  Currently they are on parallel branches:

  * 91aef03015 (refs/upstream/jk/banned) banned.h: mark ctime_r() and asctime_r() as banned
  * 1fbfdf556f banned.h: mark non-reentrant gmtime, etc as banned
  | * 4f6460df55 (refs/upstream/tb/bugreport-no-localtime) builtin/bugreport.c: use thread-safe localtime_r()
  |/  
  * 72ffeb997e Ninth batch

and because of the not-yet-removed localtime() call in
'builtin/bugreport.c' commits 1fbfdf556f and 91aef03015 can't be
built.