There’s a FreeBSD commit to telnet. fix a couple of snprintf() buffer overflows. It’s received a bit of attention for various reasons, telnet in 2019?, etc. I thought I’d take a look. Here’s a few random observations.

Here are three new lines, after the patch.

                unsigned int buflen = strlen(hbuf) + strlen(cp2) + 1;
		cp = (char *)malloc(sizeof(char)*buflen);
		snprintf((char *)cp, buflen, "%s%s", hbuf, cp2);

1. The first line is indented with spaces while the others use tabs.

2. The correct type for string length is size_t not unsigned int.

3. sizeof(char) is always one. There’s no need to multiply by it.

4. If you do need to multiply by a size, this is an unsafe pattern. Use calloc or something similar. (OpenBSD provides reallocarray to avoid zeroing cost of calloc.)

5. Return value of malloc doesn’t need to be cast. In fact, should not be, lest you disguise a warning.

6. Return value of malloc is not checked for NULL.

7. No reason to cast cp to char * when passing to snprintf. It already is that type. And if it weren’t, what are you doing?

8. The whole operation could be simplified by using asprintf.

9. Although unlikely (probably impossible here, but more generally), adding the two source lengths together can overflow, resulting in truncation with an unchecked snprintf call. asprintf avoids this failure case.