[v2,2/2] banned.h: mark ctime_r() and asctime_r() as banned.

Commit Message

Junio C Hamano Dec. 1, 2020, 9:11 p.m. UTC

From: Jeff King <[e-mail]>

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 <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 banned.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Eric Sunshine Dec. 1, 2020, 9:16 p.m. UTC | #1

On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <[e-mail]> 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=[e-mail]/

Junio C Hamano Dec. 1, 2020, 10:07 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Tue, Dec 1, 2020 at 4:12 PM Junio C Hamano <[e-mail]> 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 Dec. 1, 2020, 10:22 p.m. UTC | #3

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 <[e-mail]>

if you need it (which I doubt, but there it is anyway).


Thanks,
Taylor

Patch

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