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. _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
