From owner-svn-src-user@freebsd.org Wed Feb 3 07:21:04 2016 Return-Path: Delivered-To: svn-src-user@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BF776A98ADD for ; Wed, 3 Feb 2016 07:21:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 897613DA; Wed, 3 Feb 2016 07:21:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id C3BFED4478B; Wed, 3 Feb 2016 18:20:55 +1100 (AEDT) Date: Wed, 3 Feb 2016 18:20:54 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Garrett Cooper cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r295188 - user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools In-Reply-To: <201602030200.u1320KMh051933@repo.freebsd.org> Message-ID: <20160203172813.I898@besplex.bde.org> References: <201602030200.u1320KMh051933@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=cK4dyQqN c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=X0QvSdC6yIap9ZRWCgQA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Feb 2016 07:21:04 -0000 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