svn commit: r191330 - head/usr.bin/ncal
David Schultz
das at FreeBSD.ORG
Tue Apr 21 20:24:40 UTC 2009
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.
More information about the svn-src-head
mailing list