From owner-svn-src-all@FreeBSD.ORG Tue Apr 21 20:24:39 2009 Return-Path: Delivered-To: svn-src-all@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9CED8106566B; Tue, 21 Apr 2009 20:24:39 +0000 (UTC) (envelope-from das@FreeBSD.ORG) Received: from zim.MIT.EDU (ZIM.MIT.EDU [18.95.3.101]) by mx1.freebsd.org (Postfix) with ESMTP id 39BA98FC0C; Tue, 21 Apr 2009 20:24:39 +0000 (UTC) (envelope-from das@FreeBSD.ORG) Received: from zim.MIT.EDU (localhost [127.0.0.1]) by zim.MIT.EDU (8.14.3/8.14.2) with ESMTP id n3LKRswQ019497; Tue, 21 Apr 2009 16:27:54 -0400 (EDT) (envelope-from das@FreeBSD.ORG) Received: (from das@localhost) by zim.MIT.EDU (8.14.3/8.14.2/Submit) id n3LKRsM6019496; Tue, 21 Apr 2009 16:27:54 -0400 (EDT) (envelope-from das@FreeBSD.ORG) Date: Tue, 21 Apr 2009 16:27:54 -0400 From: David Schultz To: Robert Watson Message-ID: <20090421202754.GA19417@zim.MIT.EDU> Mail-Followup-To: Robert Watson , Roman Divacky , src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, svn-src-head@FreeBSD.ORG References: <200904201819.n3KIJcZo054306@svn.freebsd.org> <20090421185436.GA18628@zim.MIT.EDU> <20090421190651.GA2505@freebsd.org> <20090421194622.GA19215@zim.MIT.EDU> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Cc: svn-src-head@FreeBSD.ORG, Roman Divacky , src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG Subject: Re: svn commit: r191330 - head/usr.bin/ncal X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Apr 2009 20:24:40 -0000 On Tue, Apr 21, 2009, Robert Watson wrote: > On Tue, 21 Apr 2009, David Schultz wrote: > > >On Tue, Apr 21, 2009, Roman Divacky wrote: > >>>Also, before this change, ncal was already full of convoluted buffer > >>>handling, arbitrary buffer sizes, and little to no bounds checking. This > >>>commit adds more magic numbers and fragile buffer handling code, and > >>>generally makes an already hairy program even less scrutable. This isn't > >>>your fault, but it would be nice if we could make ncal better before it > >>>gets much worse. For instance, you might use snprintf() or asprintf() > >>>instead of an extra half dozen calls to memcpy() with various offsets. > >> > >>yes, thats true. do you want me to revert this? I am perfectly fine with > >>having locally modified cal that supports this highlighting and not share > >>this with world at all. > > > >I don't care (although some other people on this thread seem to); I'm just > >encouraging you to clean things up a little before making the code even > >less maintainable. > > The usual moral seems to apply: people who make cosmetic changes should > expect cosmetic criticisms. If they aren't happy to receive the criticism, > they had best refrain from the changes. Likewise modifying style(9), > making gratuitous style changes, re-spelling computer science non-words in > comments, etc. My criticism is somewhat more than cosmetic. When I fixed the multibyte handling in ncal, I fixed several buffer overflows in the process. The recent cosmetic change adds 68 lines of code, several of which look like the following: + memcpy(mlines->lines[i] + k + l + dw, term_e, + strlen(term_e)); To even understand this, you have to figure out what i, k, l, and dw refer to, and good luck trying to convince yourself that the call doesn't overflow lines[]. (Roman's argument appears to be, ``We'll just increase the buffer size from 28 to 64 and assume that's enough,'' which is probably correct but a tad sloppy.) On the other hand, I don't mean to blame Roman or insist that he fix it. When I last touched ncal, I didn't have enough time to fix all of these things either. In fact, I only converted about half of it to support multibyte month names and so forth, and strictly speaking it should use wide character output routines exclusively.