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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2020, 18:27 UTC
Message-ID: <xmqqlfehqt4n.fsf@gitster.c.googlers.com>
In-Reply-To: <X8WqFynk23yWT6E3@coredump.intra.peff.net>

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