banned.h: mark strncat() as banned
diff mbox series

Message ID 20190102093846.6664-1-e@80x24.org
State New
Headers show
Series
  • banned.h: mark strncat() as banned
Related show

Commit Message

Eric Wong Jan. 2, 2019, 9:38 a.m. UTC
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(+)

Comments

Eric Sunshine Jan. 2, 2019, 6 p.m. UTC | #1
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, 4:49 a.m. UTC | #2
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

Patch
diff mbox series

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