From owner-freebsd-audit  Mon Jul 15 13:54:27 2002
Delivered-To: freebsd-audit@freebsd.org
Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP
	id 5393937B400; Mon, 15 Jul 2002 13:54:24 -0700 (PDT)
Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16])
	by mx1.FreeBSD.org (Postfix) with ESMTP
	id 43FC143E42; Mon, 15 Jul 2002 13:54:23 -0700 (PDT)
	(envelope-from bde@zeta.org.au)
Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102])
	by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id GAA04864;
	Tue, 16 Jul 2002 06:54:20 +1000
Date: Tue, 16 Jul 2002 06:57:51 +1000 (EST)
From: Bruce Evans <bde@zeta.org.au>
X-X-Sender: bde@gamplex.bde.org
To: Giorgos Keramidas <keramida@FreeBSD.org>
Cc: Dag-Erling Smorgrav <des@ofug.org>, <freebsd-audit@FreeBSD.org>
Subject: Re: bin/ln & WARNS=5
In-Reply-To: <20020715193047.GE55859@hades.hell.gr>
Message-ID: <20020716063636.R41957-100000@gamplex.bde.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-freebsd-audit@FreeBSD.ORG
Precedence: bulk
List-ID: <freebsd-audit.FreeBSD.ORG>
List-Archive: <http://docs.freebsd.org/mail/> (Web Archive)
List-Help: <mailto:majordomo@FreeBSD.ORG?subject=help> (List Instructions)
List-Subscribe: <mailto:majordomo@FreeBSD.ORG?subject=subscribe%20freebsd-audit>
List-Unsubscribe: <mailto:majordomo@FreeBSD.ORG?subject=unsubscribe%20freebsd-audit>
X-Loop: FreeBSD.ORG

On Mon, 15 Jul 2002, Giorgos Keramidas wrote:

> Warren has suggested that we check first if the buffer space would be
> enough and then use snprintf() [as I understood from a later post].

Hi Warren ;-).  I prefer the version using snprintf() because it does
the overflow checks correctly.

> It's not very clear if snprintf() can return a negative value, and it
> is definitely possible as some have pointed out in C99.  I prefer
> guarding against this case too, just in case.

POSIX.1-2001 says some more about this and adds some bugs, at least in
the draft7 version.  It specifies setting errno to EOVERFLOW and requires

	snprintf(NULL, SIZE_MAX, "")

to fail gratuitously if SIZE_MAX > INT_MAX.  FreeBSD's snprintf() only
fails due to overflow errors if the infinite-precision result would be
larger than INT_MAX.  This can easily happen for silly cases like

	snprintf(NULL, 0, "%*sX", INT_MAX, "")

or even for

	snprintf(path, sizeof(path), "%s/%s", source, p);

where strlen(source) + 1 + strlen(p) in infinit precision is larger than
INT_MAX.  Some of the strings in ln.c are from argv, so only non-astonishing
limits on ARG_MAX prevent users providing strings that would cause overflow.

> So, we have now two versions of the diff to choose from.  One that
> checks for the appropriate buffer size before snprintf() is called.
> I've changed sizeof to use () here, and sprintf/snprintf again.
>
> %%%
> Index: ln.c
> ===================================================================
> RCS file: /home/ncvs/src/bin/ln/ln.c,v
> retrieving revision 1.28
> diff -u -r1.28 ln.c
> --- ln.c	30 Jun 2002 05:13:54 -0000	1.28
> +++ ln.c	15 Jul 2002 18:20:19 -0000
> @@ -189,12 +189,12 @@
>  			p = target;
>  		else
>  			++p;
> -		if (snprintf(path, sizeof(path), "%s/%s", source, p) >=
> -		    sizeof(path)) {
> +		if (strlen(source) + strlen(p) + 2 > sizeof(path)) {
   		    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This expression may overflow (twice).  Most code doesn't worry about such
overflows, but snprintf() avoids this overflow so I think we should depend
on snprintf()'s checking and not roll our own broken version.

>  			errno = ENAMETOOLONG;
>  			warn("%s", target);
>  			return (1);
>  		}
> +		snprintf(path, sizeof(path), "%s/%s", source, p);
>  		source = path;
>  	}
>
> %%%
>
> And the second version is something that calls snprintf() anyway and
> then checks the result.

I prefer this, except it has a new style bug (`sizeof(path)' -> `sizeof path'
in 1 place).

Bruce


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message