svn commit: r349896 - head/contrib/telnet/telnet

[Bruce Evans]

On Thu, 11 Jul 2019, Enji Cooper wrote:

>> On Jul 10, 2019, at 3:36 PM, Philip Paeps <philip at FreeBSD.org> wrote:
>>
>> Author: philip
>> Date: Wed Jul 10 22:36:14 2019
>> New Revision: 349896
>> URL: https://svnweb.freebsd.org/changeset/base/349896
>>
>> Log:
>>  telnet: fix minor style violation
>>
>>  While here also fix a very unlikely NULL pointer dereference.

I see no fix here.

>> Modified: head/contrib/telnet/telnet/commands.c
>> ==============================================================================
>> --- head/contrib/telnet/telnet/commands.c	Wed Jul 10 22:23:59 2019	(r349895)
>> +++ head/contrib/telnet/telnet/commands.c	Wed Jul 10 22:36:14 2019	(r349896)
>> @@ -45,6 +45,7 @@ __FBSDID("$FreeBSD$");
>> #include <sys/socket.h>
>> #include <netinet/in.h>
>>
>> +#include <assert.h>
>> #include <ctype.h>
>> #include <err.h>
>> #include <errno.h>
>> @@ -1654,11 +1655,13 @@ env_init(void)
>> 		|| (strncmp((char *)ep->value, "unix:", 5) == 0))) {
>> 		char hbuf[256+1];
>> 		char *cp2 = strchr((char *)ep->value, ':');
>> +                size_t buflen;

This adds a different type of style bug (indentation via spaces instead of
tabs).

>>
>> 		gethostname(hbuf, sizeof(hbuf));
>> 		hbuf[sizeof(hbuf)-1] = '\0';
>> -                unsigned int buflen = strlen(hbuf) + strlen(cp2) + 1;
>> + 		buflen = strlen(hbuf) + strlen(cp2) + 1;
>> 		cp = (char *)malloc(sizeof(char)*buflen);
>> +		assert(cp != NULL);
>
> This will unfortunately still segfault if assert is compiled out of the system as a no-op (-DNDEBUG).

telnet is unlikely to be compiled with -DNDEBUG, since it didn't use assert()
before and this commit doesn't change its Makefile to control its new use
of assert().

Any it doesn't fix the error either.  On must arches, it turns a nice
restartable null pointer trap into an unrestartable abort().  The program
crashes in both cases.

> I genuinely think using asprintf instead is the way to go, as Eitan and Warner brought up, since it reduces complexity in the program.

asprintf() is only slightly easier to use.  You still have to check if it
succeeded, and free the storage that it allocates.  It increases the
complexity of the program compared with static allocation and strcat().

Example of a simple method:

 		char buf[MAXHOSTNAMELEN + 256];
 		...

 		cp2 = ...;
 		/*
 		 * Must bound cp2's length if we are paranoid, since cp2
 		 * is in the environment.  Use 256 for the bound, for
 		 * simple portable allocation above (don't use unportable
 		 * alloca() or newfangled VLAs to try to support much
 		 * larger sizes.  Since we are paranoid, we are reluctant
 		 * to even allocate 1-byte buffers as local variables.
 		 */
 		if (strlen(cp2) > 256)
 			abort();	/* sloppy, like assert() ... */

 		/*
 		 * Although MAXHOSTNAMELEN is undocumented, we assume that
 		 * it equals the documented {HOST_NAME_MAX} + 1 elsewhere
 		 * so may as well assume this here.  We trust gethostname()
 		 * to work as documented and nul-terminate what it returns
 		 * provided it succeeds and the buffer is large enough.
 		 */
 		if (gethostname(buf, MAXHOSTNAMELEN) != 0)
 			abort();	/* sloppy, but better than nothing */

#ifdef TRUE_PARANOID
 		buf[MAXHOSTNAMELEN] = '\0';
 		assert(strlen(buf) < MAXHOSTNAMELEN);
#endif

 		/* The buffer is large enough, by construction. */
 		strcat(buf, cp2);

The above is much more readable without any comments or error checking.  I
can't quite make it fail nicely with null pointer traps then:

 		buf = alloca(MAXHOSTNAMELEN + strlen(cp2));
 		gethostname(buf, MAXHOSTNAMELEN);
 		strcat(buf, cp2);

Using alloca() is better than a VLA, since it its possible in principle for
it to fail and return a null pointer giving a nice trap.

Using asprintf():

 		/* Minor unportabilities, as above: */
 		char hbuf[MAXHOSTNAMELEN];

 		/*
 		 * Only needs error checking if TRUE_PARANOID, as above
 		 * (might be better to order the initialization so as to
 		 * use out global hostname variable):
 		 */
 		gethostname(hbuf, MAXHOSTNAMELEN);

 		/* Remove cp2 above; expression for cp2 moved here: */
 		asprintf(&buf, "%s%s", buf, strchr((char *)ep->value, ':'));
 		if (buf == NULL)
 			abort();	/* sloppy, like assert() ... */
 		...
 		free(buf);		/* more messes to free it */

xasprintfa() (asprintf() to an alloca()ed buffer with internal abort() of
the allocation is impossible) would be most convenient here.

Bruce