Date: Tue, 16 Jul 2002 05:14:10 +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: <20020716044254.G41571-100000@gamplex.bde.org> In-Reply-To: <20020715111436.GD50130@hades.hell.gr>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 15 Jul 2002, Giorgos Keramidas wrote:
> On 2002-07-15 20:25 +0000, Bruce Evans wrote:
> > On 15 Jul 2002, Dag-Erling Smorgrav wrote:
> >
> > > Giorgos Keramidas <keramida@FreeBSD.org> writes:
> > > > The following allows me to build bin/ln with WARNS=5 on i386.
> > > > Does it look OK, or have I missed something important?
> > >
> > > I'd rather cast sizeof to int.
> >
> > That would break the possibly-intentional check for snprintf() failing.
> > (size_t)-1 >= sizeof(path), but !(-1 >= (int)sizeof(path)).
>
> My intuition was that size_t being unsigned won't require truncation
> of the (int) return value...
Lack of truncation by either of these casts is not at all clear. (This is
true of casts in general, which is why they should be avoided. But the
original code has implicit casts.)
However, snprintf() is specified to return "a negative value" on error,
and casting a negative calue to size_t might truncate it a lot, e.g.,
to 0 on machines with 33-bit ints and 32-bit u_ints.
Documenttion of failure of snprintf is completely missing in printf.3.
> But if one wanted to explicitly make
> both a check for (-1) and the return value being less than the size of
> the buffer would the following be more proper?
One should want to check for a negative value.
> %%%
> 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 11:12:13 -0000
> @@ -163,6 +163,7 @@
> const char *p;
> int ch, exists, first;
> char path[PATH_MAX];
> + int pathlen;
This unsorts the variables.
>
> if (!sflag) {
> /* If target doesn't exist, quit now. */
> @@ -189,8 +190,8 @@
> p = target;
> else
> ++p;
> - if (snprintf(path, sizeof(path), "%s/%s", source, p) >=
> - sizeof(path)) {
> + if ((pathlen = snprintf(path, sizeof(path), "%s/%s",
> + source, p)) == -1 || pathlen >= (int)sizeof(path)) {
> errno = ENAMETOOLONG;
> warn("%s", target);
> return (1);
> %%%
This takes more code than I like, but I guess something like it is required
to be strictly correct. I prefer:
pathlen = snprintf(path, sizeof(path), "%s/%s", source, p);
if (pathlen < 0 || pathlen >= (int)sizeof(path))) {
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?20020716044254.G41571-100000>