Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 16 Jul 2002 06:57:51 +1000 (EST)
From:      Bruce Evans <bde@zeta.org.au>
To:        Giorgos Keramidas <keramida@FreeBSD.org>
Cc:        Dag-Erling Smorgrav <des@ofug.org>, <freebsd-audit@FreeBSD.org>
Subject:   Re: bin/ln & WARNS=5
Message-ID:  <20020716063636.R41957-100000@gamplex.bde.org>
In-Reply-To: <20020715193047.GE55859@hades.hell.gr>

next in thread | previous in thread | raw e-mail | index | archive | help
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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020716063636.R41957-100000>