From owner-freebsd-security  Thu Sep  7 06:48:46 1995
Return-Path: security-owner
Received: (from majordom@localhost)
          by freefall.freebsd.org (8.6.11/8.6.6) id GAA20046
          for security-outgoing; Thu, 7 Sep 1995 06:48:46 -0700
Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.34])
          by freefall.freebsd.org (8.6.11/8.6.6) with ESMTP id GAA20040
          for <security@freebsd.org>; Thu, 7 Sep 1995 06:48:42 -0700
Received: (from bde@localhost) by godzilla.zeta.org.au (8.6.9/8.6.9) id XAA04311; Thu, 7 Sep 1995 23:46:14 +1000
Date: Thu, 7 Sep 1995 23:46:14 +1000
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199509071346.XAA04311@godzilla.zeta.org.au>
To: bde@zeta.org.au, pst@shockwave.com
Subject: Re: diffs for syslog.c
Cc: security@freebsd.org
Sender: security-owner@freebsd.org
Precedence: bulk

>  >+#define	SPACELEFT(buffer, current) (sizeof (buffer) - \
>  >+				    ((char *)(current) - (char *)(buffer)))
>  
>  This is wrong if size_t is larger than ptrdiff_t.

>Thanks for the code review,  I think we have some disagreements.  I don't
>have an ANSI spec in front of me,  so please bear with me if I'm being a
>fool.

>It is my impression that the standard would have the pointer expression
>evaluated first.  I believe the result of the pointer arithmetic here is
>an integer offset, not a pointer.

>This signed integer value would be promoted to type size_t for an arithmetic
>expression.

Correct so far.  If sizeof(buffer) is (unsigned)2048, and the pointer
difference is (int)2049, then the result is (unsigned)(-1) = UINT_MAX.
Thus SPACELEFT() can say that there is an "infinite" amount of space
when there is actually none.

>  >+#define OVERFLOW(buffer, current)  ((current) > \
>  >+				    (char *)(buffer) + sizeof (buffer))
>  						     ^
>  Anal code should check for overflow here.

>OK, yes, if buffer really was at the top of memory space, you're absolutely
>correct.  How would you propose to code that overflow check?

Oops, the overflow possibility is in the `+' for the evaluation of
`current', not for the fixed buffer.  Buffers are not allowed to be
allocated at the end of memory in Standard C.  Otherwise it would
be even harder to write these checks portably.

>  In fact, the check can't be
>  written like this in Standard C.  `fooptr + offset' is undefined if the
>  offset isn't a valid array index (or maybe one larger).

>I disagree.  Performing a dereference of the value is undefined, but the
>pointer arithmetic is still valid and defined.

Well, it isn't.  Consider the simplest case of a linear address space
when the addition overflows.  The usual behaviour is to wrap but the
"right" behaviour is to trap.  The standard correctly refrains from
specifying either behaviour.

>  The correct test is something like
>  
>  	int nwritten;
>  	ssize_t spaceleft;
>  	...
>  	spaceleft = SPACELEFT(...);
>  	assert(spaceleft >= 0);
>  	nwritten = vsnprintf(..., spaceleft, ...);
>  	assert(nwritten >= 0);

>When would you see nwritten return less than 0?  It's a signed integer,
>and the 'error' case is 0.

There is no error case for [v]s[n]printf() AFAIK.  I was thinking of
vnsprintf() returning EOF although this "can't happen".  Actually the
following returns less than 0 (INT_MIN) in FreeBSD-current:

	snprintf(buf, 1, "%*s%*s", INT_MAX, "", 1, "");

(this takes 322 seconds on a 486DX2/66), and the following returns 0:

	snprintf(buf, 2, "%*s%*s%*s", INT_MAX, "foo", INT_MAX, "", 2, "");

It leaves buf[0] as '\0' instead of 'f'.  I think it writes 'f' at first
and wraps to write '\0' out of the last string.

vsprintf() expects to accumulate the return value in an `int'.  It
deserves to be terminated by an overflow trap.

BTW, `gcc -Wformat' doesn't know about [v]snprintf().

Bruce