threads / patch / 54734

patchbuiltin/bugreport.c: use thread-safe localtime_r()

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

## tl;dr

14 messages between Nov 30, 2020 and Dec 6, 2020. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

Taylor Blau· Nov 30, 2020, 23:06 UTC · lore

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(-)
Show changes to builtin/bugreport.c +2 −2
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
Taylor Blau· Dec 1, 2020, 00:30 UTC · re: Taylor Blau · lore

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

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(-)
Show changes to builtin/bugreport.c +2 −2
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
Jeff King· Dec 1, 2020, 02:27 UTC · re: Taylor Blau · lore

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

On Mon, Nov 30, 2020 at 07:30:06PM -0500, Taylor Blau wrote:
Show 7 quoted lines
> @@ -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(+)
Show changes to banned.h +13 −0
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
Eric Sunshine· Dec 1, 2020, 03:15 UTC · re: Jeff King · lore

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

On Mon, Nov 30, 2020 at 9:30 PM Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> 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.
Junio C Hamano· Dec 1, 2020, 18:27 UTC · re: Jeff King · lore

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

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.
Show 44 quoted lines
> 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 */
Taylor Blau· Dec 1, 2020, 18:34 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Dec 1, 2020, 21:11 UTC · re: Taylor Blau · lore

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

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(+)
Show changes to banned.h +9 −0
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
Junio C Hamano· Dec 1, 2020, 21:11 UTC · re: Junio C Hamano · lore

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

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(+)
Show changes to banned.h +4 −0
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
Eric Sunshine· Dec 1, 2020, 21:16 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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/
Junio C Hamano· Dec 1, 2020, 22:07 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 9 quoted lines
> 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.
Taylor Blau· Dec 1, 2020, 22:22 UTC · re: Junio C Hamano · lore

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

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

SZEDER Gábor· Dec 6, 2020, 14:56 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 01, 2020 at 01:11:37PM -0800, Junio C Hamano wrote:
Show 33 quoted lines
> 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.

Jeff King· Dec 2, 2020, 01:57 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> 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
Eric Sunshine· Dec 1, 2020, 00:31 UTC · re: Taylor Blau · lore

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

On Mon, Nov 30, 2020 at 6:10 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 15 quoted lines
> 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.

← back to recent threads