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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 1, 2020, 00:31 UTC
Message-ID: <CAPig+cRx03potus-ea-4J8mCuG3vVeQBJ8NcEh_Hs2yJqaoXcw@mail.gmail.com>
In-Reply-To: <27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com>

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.