threads / patch / 50144

patchbanned.h: mark strncat() as banned

Subject: [PATCH] banned.h: mark strncat() as banned

## tl;dr

3 messages between Jan 2, 2019 and Jan 3, 2019. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Eric Wong· Jan 2, 2019, 09:38 UTC · lore

strncat() has the same quadratic behavior as strcat() and is difficult-to-read and bug-prone. While it hasn't yet been a problem in git iself, strncat() found it's way into 'master' of cgit and caused segfaults on my system.

Signed-off-by: Eric Wong <e@80x24.org>
---
 banned.h | 2 ++
 1 file changed, 2 insertions(+)

Show changes to banned.h +2 −0

diff --git a/banned.h b/banned.h
index 28f5937035..447af24807 100644
--- a/banned.h
+++ b/banned.h
@@ -16,6 +16,8 @@
 #define strcat(x,y) BANNED(strcat)
 #undef strncpy
 #define strncpy(x,y,n) BANNED(strncpy)
+#undef strncat
+#define strncat(x,y,n) BANNED(strncat)
 
 #undef sprintf
 #undef vsprintf

-- 
EW

Eric Sunshine· Jan 2, 2019, 18:00 UTC · re: Eric Wong · lore

Re: [PATCH] banned.h: mark strncat() as banned

On Wed, Jan 2, 2019 at 4:38 AM Eric Wong <e@80x24.org> wrote:

>
> strncat() has the same quadratic behavior as strcat() and is
> difficult-to-read and bug-prone.  While it hasn't yet been a
> problem in git iself, strncat() found it's way into 'master'

s/iself/itself/

> of cgit and caused segfaults on my system.
>
> Signed-off-by: Eric Wong <e@80x24.org>

Jeff King· Jan 3, 2019, 04:49 UTC · re: Eric Wong · lore

Re: [PATCH] banned.h: mark strncat() as banned

On Wed, Jan 02, 2019 at 09:38:46AM +0000, Eric Wong wrote:

> strncat() has the same quadratic behavior as strcat() and is
> difficult-to-read and bug-prone.  While it hasn't yet been a
> problem in git iself, strncat() found it's way into 'master'
> of cgit and caused segfaults on my system.

I'm in favor of this.

It doesn't have the "oops, I didn't NUL-terminate for you" problem that strncpy() has. But it actually has the opposite problem! It will always place a NUL, and you have to feed it sizeof(dst)-1 to avoid an overflow.

So I think it's important for safety (though I'd be fine banning it on the quadratic grounds alone ;) ).

-Peff