From owner-cvs-lib  Mon Mar 17 13:22:07 1997
Return-Path: <owner-cvs-lib>
Received: (from root@localhost)
          by freefall.freebsd.org (8.8.5/8.8.5) id NAA05225
          for cvs-lib-outgoing; Mon, 17 Mar 1997 13:22:07 -0800 (PST)
Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19])
          by freefall.freebsd.org (8.8.5/8.8.5) with ESMTP id NAA05220;
          Mon, 17 Mar 1997 13:21:54 -0800 (PST)
Received: (from bde@localhost) by godzilla.zeta.org.au (8.8.3/8.6.9) id IAA27478; Tue, 18 Mar 1997 08:07:04 +1100
Date: Tue, 18 Mar 1997 08:07:04 +1100
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199703172107.IAA27478@godzilla.zeta.org.au>
To: eivind@freefall.freebsd.org, guido@gvr.win.tue.nl
Subject: Re: cvs commit:  src/lib/libtermcap tgoto.c
Cc: cvs-all@freefall.freebsd.org, CVS-committers@freefall.freebsd.org,
        cvs-lib@freefall.freebsd.org
Sender: owner-cvs-lib@FreeBSD.ORG
X-Loop: FreeBSD.org
Precedence: bulk

>>   Modified:    lib/libtermcap  tgoto.c
>>   Log:
>>   Buffer overflow fix - closes PR bin/2983 for -current.  Should really
>>   go into 2.2.0 Release, even at the present time.  Problem spotted by
>>   Tero Kivinen <kivinen@ssh.fi> - was in BugTraq today :-(
>
>Shouln't you \0-terminate the copied string? This was suggested in the
>same article.

The string is terminated by strcpy.  The article was talking about an
allegedly bad fix that used strncpy.  strncpy is OK if the final byte
of the array is never changed from its intial value of 0.

>Further, there is a strcpy on the end. That should also be fixed.

No, it is correct, except for a minor bug in the overflow checking.
I sent private mail to Eivind about it.

>I think the if statements should be something like:
>if (dp >= &result[MAXRETURNSIZE-1])

This would ensure space for the terminator, but the check at the end
already does that.

>The strpcy should be:
>
>strncpy(dp, added, sizeof(result) - (dp - result) - 1);

Truncation would be worse than returning the obviously bogus result
"OVERFLOW".

>The '\0' will automatically be always in place because it is in the bss
>and it's never overwritten.

strncpy() might be faster, but it's hard to tell.  If speed is really
important (and it probably is), then tgoto() should have avoided the
strcpy() at the end in the common case where (added[0] == 0).  Apart from
that, I prefer to keep the tests simpler by avoiding +-1 terms.

Bruce