| Message ID | 73eb4965807ea2fdf94f815a8f8a2b036296ecca.1606782566.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] builtin/bugreport.c: use thread-safe localtime_r() | expand |
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 <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 */
On Mon, Nov 30, 2020 at 9:30 PM Jeff King <peff@peff.net> 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.
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. > 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 */
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
On Tue, Dec 01, 2020 at 10:27:20AM -0800, Junio C Hamano wrote: > 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
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)) {
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(-) -- 2.29.2.533.g07db1f5344