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>