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

Commit Message

Taylor Blau Nov. 30, 2020, 11:06 p.m. UTC

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(-)

--
2.29.2.533.g07db1f5344

Comments

Eric Sunshine Dec. 1, 2020, 12:31 a.m. UTC | #1

On Mon, Nov 30, 2020 at 6:10 PM Taylor Blau <[e-mail]> 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.

Patch

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)) {