From owner-svn-src-head@freebsd.org Tue Aug 4 06:12:31 2015 Return-Path: Delivered-To: svn-src-head@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 CCED19B2BB6; Tue, 4 Aug 2015 06:12:31 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 940A7DAE; Tue, 4 Aug 2015 06:12:31 +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 mail104.syd.optusnet.com.au (Postfix) with ESMTPS id E53A1425D74; Tue, 4 Aug 2015 16:12:20 +1000 (AEST) Date: Tue, 4 Aug 2015 16:12:19 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286268 - head/usr.bin/wall In-Reply-To: <201508040256.t742uWfQ053686@repo.freebsd.org> Message-ID: <20150804155237.L1126@besplex.bde.org> References: <201508040256.t742uWfQ053686@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=eZjABOwH c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=JEayULNB4bDuRC7VNgQA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Aug 2015 06:12:31 -0000 On Tue, 4 Aug 2015, Pedro F. Giffuni wrote: > Log: > Revert r286144 leaving the original fix to the buffer overflow. > > Some developers consider the new code unnecessarily obfuscated. > There was also a benign off-by-one. > > Discussed with: bde, vangyzen, jmallett It is this version that is unnecessarily obfuscated. > Modified: head/usr.bin/wall/ttymsg.c > ============================================================================== > --- head/usr.bin/wall/ttymsg.c Tue Aug 4 02:41:14 2015 (r286267) > +++ head/usr.bin/wall/ttymsg.c Tue Aug 4 02:56:31 2015 (r286268) > @@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co > struct iovec localiov[7]; > ssize_t left, wret; > int cnt, fd; > - char device[MAXNAMLEN]; > + char device[MAXNAMLEN] = _PATH_DEV; > static char errbuf[1024]; > char *p; > int forked; Half of the string initialization is done by an auto initializer. Initializations in declarations are specifically forbidden in style(9), and this is a good example of how to obfuscate code using them. The original version using a static initializer was almost as obfuscated, but it was at least maximally efficient and had a technical reason for using the initializer. > @@ -71,9 +71,8 @@ ttymsg(struct iovec *iov, int iovcnt, co > if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0]))) > return ("too many iov's (change code in wall/ttymsg.c)"); > > - strlcpy(device, _PATH_DEV, sizeof(device)); > + strlcat(device, line, sizeof(device)); The other half of the initialization is done using strlcat(). > p = device + sizeof(_PATH_DEV) - 1; > - strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV)); > if (strncmp(p, "pts/", 4) == 0) > p += 4; > if (strchr(p, '/') != NULL) { In private mail, I pointed out lots more bad code here. The pointer 'p' was introdiced to avoid writing out the expression for it in more places, especially in the "pts/" check. But the "pts/" check is badly written. It checks the copy of 'line' in 'device'. This is an obfuscated way of checking 'line' itself. It might be more secure the check the final string, but it is actually more secure to check 'line' since this will detect slashes that lost by strlcat() with no error checking. This method becomes even more unnatural when combined with the strcat(). We use the knowledge of the prefix to find the place to start for the check, but don't use this to find the place to start for the concatenation. These places are the same, and unwinding the obfuscation in the check requires knowing that they are the same. Bruce