# [PATCH] builtin/bugreport.c: use thread-safe localtime_r() 14 messages from 2020-11-30 to 2020-12-06. Participants: Taylor Blau, Eric Sunshine, Jeff King, Junio C Hamano, SZEDER Gábor. Thread: https://gitlist.dev/t/54734 ## Taylor Blau, 2020-11-30 23:06 Subject: [PATCH] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: <27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com> URL: https://gitlist.dev/e/27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me%40ttaylorr.com ``` 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 ``` ## Taylor Blau, 2020-12-01 00:30 Subject: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: <73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me@ttaylorr.com> URL: https://gitlist.dev/e/73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me%40ttaylorr.com In-Reply-To: <27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com> ``` 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 --- 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 ``` ## Eric Sunshine, 2020-12-01 00:31 Subject: Re: [PATCH] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRx03potus-ea-4J8mCuG3vVeQBJ8NcEh_Hs2yJqaoXcw%40mail.gmail.com In-Reply-To: <27fc158339c91f56210f00dae9015da1d6c781ec.1606777520.git.me@ttaylorr.com> ``` On Mon, Nov 30, 2020 at 6:10 PM Taylor Blau 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. ``` ## Jeff King, 2020-12-01 02:27 Subject: Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/X8WqFynk23yWT6E3%40coredump.intra.peff.net In-Reply-To: <73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me@ttaylorr.com> ``` 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 --- 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 ``` ## Eric Sunshine, 2020-12-01 03:15 Subject: Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT%3DgMEuKkbJefT9yxWWB5VC1fj6T%2Bofjn_saEEeEeU_MA%40mail.gmail.com In-Reply-To: ``` On Mon, Nov 30, 2020 at 9:30 PM Jeff King 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. ``` ## Junio C Hamano, 2020-12-01 18:27 Subject: Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/xmqqlfehqt4n.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jeff King 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 > --- > 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, 2020-12-01 18:34 Subject: Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/X8aMt2LEiCLkdV9%2F%40nand.local In-Reply-To: ``` 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, 2020-12-01 21:11 Subject: [PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned Message-ID: <20201201211138.33850-1-gitster@pobox.com> URL: https://gitlist.dev/e/20201201211138.33850-1-gitster%40pobox.com In-Reply-To: ``` From: Jeff King 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 Signed-off-by: Junio C Hamano --- 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 ``` ## Junio C Hamano, 2020-12-01 21:11 Subject: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned. Message-ID: <20201201211138.33850-2-gitster@pobox.com> URL: https://gitlist.dev/e/20201201211138.33850-2-gitster%40pobox.com In-Reply-To: <20201201211138.33850-1-gitster@pobox.com> ``` From: Jeff King 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 Signed-off-by: Junio C Hamano --- 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 ``` ## Eric Sunshine, 2020-12-01 21:16 Subject: Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned. Message-ID: URL: https://gitlist.dev/e/CAPig%2BcROG5%2BkhWvBWbWgVhNuDyWkCQYBXwte5VeazuCCXMAA_g%40mail.gmail.com In-Reply-To: <20201201211138.33850-2-gitster@pobox.com> ``` On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano 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/ ``` ## Junio C Hamano, 2020-12-01 22:07 Subject: Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned. Message-ID: URL: https://gitlist.dev/e/xmqqzh2xmb7o.fsf%40gitster.c.googlers.com In-Reply-To: ``` Eric Sunshine writes: > On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano 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, 2020-12-01 22:22 Subject: Re: [PATCH v2 2/2] banned.h: mark ctime_r() and asctime_r() as banned. Message-ID: URL: https://gitlist.dev/e/X8bCJ%2FZukppAsoTY%40nand.local In-Reply-To: ``` 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 if you need it (which I doubt, but there it is anyway). Thanks, Taylor ``` ## Jeff King, 2020-12-02 01:57 Subject: Re: [PATCH v2] builtin/bugreport.c: use thread-safe localtime_r() Message-ID: URL: https://gitlist.dev/e/X8b0he9VVI5s1log%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote: > Jeff King 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 ``` ## SZEDER Gábor, 2020-12-06 14:56 Subject: Re: [PATCH v2 1/2] banned.h: mark non-reentrant gmtime, etc as banned Message-ID: <20201206145642.GH8396@szeder.dev> URL: https://gitlist.dev/e/20201206145642.GH8396%40szeder.dev In-Reply-To: <20201201211138.33850-1-gitster@pobox.com> ``` On Tue, Dec 01, 2020 at 01:11:37PM -0800, Junio C Hamano wrote: > From: Jeff King > > 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 > Signed-off-by: Junio C Hamano > --- > 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. ```