Date: Wed, 3 Feb 2016 18:20:54 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <ngie@freebsd.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r295188 - user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools Message-ID: <20160203172813.I898@besplex.bde.org> In-Reply-To: <201602030200.u1320KMh051933@repo.freebsd.org> References: <201602030200.u1320KMh051933@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 3 Feb 2016, Garrett Cooper wrote: > Log: > Use destination buffer instead of source buffer size to mute valid > security concerns with strlcpy related to their respective buffer > sizes (-Wstrlcpy-strlcat-size) Why not fix the bug instead of not hear the warning about it? The change might actually be a fix, but the log message makes it sound like it isn't. > ============================================================================== > --- user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmpmap.c Wed Feb 3 01:58:37 2016 (r295187) > +++ user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmpmap.c Wed Feb 3 02:00:20 2016 (r295188) > @@ -283,7 +283,7 @@ enum_pair_insert(struct enum_pairs *head > } > > e_new->enum_val = enum_val; > - strlcpy(e_new->enum_str, enum_str, strlen(enum_str) + 1); > + strlcpy(e_new->enum_str, enum_str, strlen(e_new->enum_str)); > STAILQ_INSERT_TAIL(headp, e_new, link); > > return (1); This actually completely breaks the code. It applies strlen() to garbage memory that was just allocated to hold the string. The old code was correct except being obfuscated using strlcpy(). > @@ -569,7 +569,7 @@ snmp_enumtc_init(char *name) > free(enum_tc); > return (NULL); > } > - strlcpy(enum_tc->name, name, strlen(name) + 1); > + strlcpy(enum_tc->name, name, sizeof(enum_tc->name)); > > return (enum_tc); > } This also completely breaks the code. It uses sizeof on the pointer to the memory that will become the new string when it is initialized. The old code was correct except being obfuscated using strlcpy(). These bugs show why strlen() shouldn't exist. Its use in the old code was just an obfuscation. It only looks wrong because its return value is not checked, so it looks like truncation is not detected. However, since the buffers were just allocated with the correct size, truncation is impossible and ordinary strcpy() works perfectly. The unbroken version was essentially a home made version of strdup(). Using strdup() would be slightly easier. But I don't like it either. Using strdup() saves having to write a couple of strlen() expressions and on strcpy() statement and prevents obfuscating the strcpy() as an unchecked strlcpy(). But this is easy and I like it to be explicit. The hard part is the error handling, especially in libraries. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160203172813.I898>