Date: Tue, 18 Mar 1997 08:07:04 +1100 From: Bruce Evans <bde@zeta.org.au> To: eivind@freefall.freebsd.org, guido@gvr.win.tue.nl Cc: cvs-all@freefall.freebsd.org, CVS-committers@freefall.freebsd.org, cvs-lib@freefall.freebsd.org Subject: Re: cvs commit: src/lib/libtermcap tgoto.c Message-ID: <199703172107.IAA27478@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199703172107.IAA27478>