From owner-svn-src-all@FreeBSD.ORG Tue Apr 21 20:32:34 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 E21BD1065697; Tue, 21 Apr 2009 20:32:34 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from vlakno.cz (77-93-215-190.static.masterinter.net [77.93.215.190]) by mx1.freebsd.org (Postfix) with ESMTP id 6BF798FC18; Tue, 21 Apr 2009 20:32:33 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from localhost (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id 6ED9A9CB0A7; Tue, 21 Apr 2009 22:31:18 +0200 (CEST) X-Virus-Scanned: amavisd-new at vlakno.cz Received: from vlakno.cz ([127.0.0.1]) by localhost (lev.vlakno.cz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id g5Cv5KaOP61B; Tue, 21 Apr 2009 22:31:06 +0200 (CEST) Received: from vlk.vlakno.cz (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id 7A2D79CB112; Tue, 21 Apr 2009 22:31:06 +0200 (CEST) Received: (from rdivacky@localhost) by vlk.vlakno.cz (8.14.3/8.14.3/Submit) id n3LKV6cV014293; Tue, 21 Apr 2009 22:31:06 +0200 (CEST) (envelope-from rdivacky) Date: Tue, 21 Apr 2009 22:31:06 +0200 From: Roman Divacky To: Robert Watson , src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, svn-src-head@FreeBSD.ORG Message-ID: <20090421203106.GA13661@freebsd.org> References: <200904201819.n3KIJcZo054306@svn.freebsd.org> <20090421185436.GA18628@zim.MIT.EDU> <20090421190651.GA2505@freebsd.org> <20090421194622.GA19215@zim.MIT.EDU> <20090421202754.GA19417@zim.MIT.EDU> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090421202754.GA19417@zim.MIT.EDU> User-Agent: Mutt/1.4.2.3i Cc: 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:32:35 -0000 On Tue, Apr 21, 2009 at 04:27:54PM -0400, David Schultz wrote: > 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: there were complains about your change being incorrect which you ignored. (iirc you ignored mails from Christoph Mallon and I had to forward it to you) > + 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.) yes, it's fragile, ugly and bad.... the whole code of cal should be rewritten but frankly... program for displaying calendar is not worth the time of anyone but undergrad doing his homework > 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. feel free to work on it, or anyone else. I wanted to see what day is today when I am filling in my job reports. I got this and I dont care anymore about cal... roman