Date: Thu, 7 Sep 1995 23:46:14 +1000 From: Bruce Evans <bde@zeta.org.au> To: bde@zeta.org.au, pst@shockwave.com Cc: security@freebsd.org Subject: Re: diffs for syslog.c Message-ID: <199509071346.XAA04311@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
> >+#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199509071346.XAA04311>