svn commit: r286268 - head/usr.bin/wall

[Bruce Evans]

On Tue, 4 Aug 2015, Pedro F. Giffuni wrote:

> Log:
>  Revert r286144 leaving the original fix to the buffer overflow.
>
>  Some developers consider the new code unnecessarily obfuscated.
>  There was also a benign off-by-one.
>
>  Discussed with:	bde, vangyzen, jmallett

It is this version that is unnecessarily obfuscated.

> Modified: head/usr.bin/wall/ttymsg.c
> ==============================================================================
> --- head/usr.bin/wall/ttymsg.c	Tue Aug  4 02:41:14 2015	(r286267)
> +++ head/usr.bin/wall/ttymsg.c	Tue Aug  4 02:56:31 2015	(r286268)
> @@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co
> 	struct iovec localiov[7];
> 	ssize_t left, wret;
> 	int cnt, fd;
> -	char device[MAXNAMLEN];
> +	char device[MAXNAMLEN] = _PATH_DEV;
> 	static char errbuf[1024];
> 	char *p;
> 	int forked;

Half of the string initialization is done by an auto initializer.
Initializations in declarations are specifically forbidden in style(9),
and this is a good example of how to obfuscate code using them.  The
original version using a static initializer was almost as obfuscated,
but it was at least maximally efficient and had a technical reason
for using the initializer.

> @@ -71,9 +71,8 @@ ttymsg(struct iovec *iov, int iovcnt, co
> 	if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0])))
> 		return ("too many iov's (change code in wall/ttymsg.c)");
>
> -	strlcpy(device, _PATH_DEV, sizeof(device));
> +	strlcat(device, line, sizeof(device));

The other half of the initialization is done using strlcat().

> 	p = device + sizeof(_PATH_DEV) - 1;
> -	strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV));
> 	if (strncmp(p, "pts/", 4) == 0)
> 		p += 4;
> 	if (strchr(p, '/') != NULL) {

In private mail, I pointed out lots more bad code here.  The pointer
'p' was introdiced to avoid writing out the expression for it in
more places, especially in the "pts/" check.  But the "pts/" check
is badly written.  It checks the copy of 'line' in 'device'.  This
is an obfuscated way of checking 'line' itself.  It might be more
secure the check the final string, but it is actually more secure
to check 'line' since this will detect slashes that lost by strlcat()
with no error checking.

This method becomes even more unnatural when combined with the strcat().
We use the knowledge of the prefix to find the place to start for the
check, but don't use this to find the place to start for the concatenation.
These places are the same, and unwinding the obfuscation in the check
requires knowing that they are the same.

Bruce