Date: Tue, 21 Apr 2009 16:27:54 -0400 From: David Schultz <das@FreeBSD.ORG> To: Robert Watson <rwatson@FreeBSD.ORG> Cc: svn-src-head@FreeBSD.ORG, Roman Divacky <rdivacky@FreeBSD.ORG>, src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG Subject: Re: svn commit: r191330 - head/usr.bin/ncal Message-ID: <20090421202754.GA19417@zim.MIT.EDU> In-Reply-To: <alpine.BSF.2.00.0904212046360.67705@fledge.watson.org> References: <200904201819.n3KIJcZo054306@svn.freebsd.org> <20090421185436.GA18628@zim.MIT.EDU> <20090421190651.GA2505@freebsd.org> <20090421194622.GA19215@zim.MIT.EDU> <alpine.BSF.2.00.0904212046360.67705@fledge.watson.org>
index | next in thread | previous in thread | raw e-mail
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.home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090421202754.GA19417>