svn commit: r191330 - head/usr.bin/ncal

[David Schultz]

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.