"Secure coding" audit checkers and Kerberos

[John Hascall]

> If we were to use strlcpy and strlcat in Kerberos, I would expect to
> have unchecked usages when it is easy to tell that an overflow can't
> occur.  Most of our string manipulation code uses allocated buffers, so
> I would expect to see code like:
> 
>   buflen = strlen(s1) + strlen(s2) + strlen(s3) + 1;
>   buf = malloc(buflen);
>   if (!buf) return ENOMEM;
>   (void) strlcpy(buf, s1, buflen);
>   (void) strlcat(buf, s2, buflen);
>   (void) strlcat(buf, s3, buflen);
> 
> Adding an unreachable, untestable error path after each strl*() call in
> this situation would expand the code size, could force the use of
> goto-style exception handling in functions which don't otherwise need
> it, and would inevitably introduce phantom resource leaks which take
> time to fix.

What about hiding that sort of thing in a helper function?
something along the lines of:

char * vconcat (
	char *	s1,
	...
) {
	va_list	ap;
	size_t	len	= 1;
	size_t	off	= 0;
	char *	s;
	char *	retval;

	va_start(ap, s1);
	for (s = s1; s != NULL; s = va_arg(ap, char *)) {
		/* XXX: check for overflow of len left as exercise for reader */
		len += strlen(s);
	}
	va_end(ap);
	if ((retval = malloc(len)) == NULL) return NULL;
	va_start(ap, s1);
	for (s = s1; s != NULL; s = va_arg(ap, char *)) {
		len = strlen(s);
		/* XXX: or strlcpy if that's your fancy */
		/* XXX: or you could do a byte-by-byte copy to save a pass */
		memcpy(retval + off, s, len);
		off += len;
	}
	retval[off] = '\0';
	return retval;
}

then your usage, as shown above, becomes quite clean:

	if ((buf = vconcat(s1, s2, s3, NULL)) == NULL) return ENOMEM;


John
PS, I put about 3 minutes into typing that, it may not be perfect,
    but I trust it gets the idea across.