possible buffer overflow in tkcanvas?

[Jun T. <tak...@kb...>]

With the CVS HEAD, clang gives the following warning (and suggestion):

../term/tkcanvas.trm:694:33: warning: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Wstrncat-size]
                strncat(tmp_dashpattern, buf, sizeof(tmp_dashpattern) - strlen(tmp_dashpattern));
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../term/tkcanvas.trm:694:33: note: change the argument to be the free space in the destination buffer minus the terminating null byte
                strncat(tmp_dashpattern, buf, sizeof(tmp_dashpattern) - strlen(tmp_dashpattern));
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                              sizeof(tmp_dashpattern) - strlen(tmp_dashpattern) - 1

So I looked into the function TK_dashtype() and noticed two more
suspicious code (in addition to the line 694 pointed out by clang).
I attached a patch, but I haven't done any tests (I don't know
how to use tkcanvas). So please consider my patch just as a suggestion.

Lines 694 and 685 may be dangerous in the following sense:

If strlen(src) >=n,
  strncat(dest, src, n) adds n chars AND a trailing NUL to dest.
  strncpy(dest, src, n) copies n chars to dest but does not NUL
  terminate it.

So the safest way of using these functions is:
  strncat(dest, src, sizeof(dest) - strlen(dest) - 1);
and
  strncpy(dest, src, sizeof(dest) - 1);
  dest[sizeof(dest)-1] = NUL;

(line 685 is safe in the present case since dstring is shorter than
tmp_dashpattern, but ...)


And what does the line 696
    tmp_dashpattern[strlen(tmp_dashpattern) - 1] = NUL;
want to achieve?
Maybe a simple confusion between strncat and strncpy?


BTW, are 32 bytes really necessary for buf[]? (line 690).
sizeof(tmp_dashpattern) = 3*DASHPATTERN_LENGTH = 24 < 32
(if DASHPATTERN_LENGTH = 8).