Date: Fri, 26 Apr 2002 22:45:06 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: "Kenneth D. Merry" <ken@kdm.org> Cc: "M. Warner Losh" <imp@village.org>, <obrien@FreeBSD.org>, <cvs-committers@FreeBSD.org>, <cvs-all@FreeBSD.org> Subject: Re: cvs commit: src/lib/libcam camlib.h Message-ID: <20020426222203.P4255-100000@gamplex.bde.org> In-Reply-To: <20020425201835.A55111@panzer.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 25 Apr 2002, Kenneth D. Merry wrote:
> On Fri, Apr 26, 2002 at 05:47:12 +1000, Bruce Evans wrote:
> > On Wed, 24 Apr 2002, Kenneth D. Merry wrote:
> > > I think the attached patch will fix the problem, let me know what you
> > > think. strncpy() could be used instead of strlcpy(), with the addition of
> > > an extra line to NUL terminate the string in case the string copied into
> > > the buffer is as long as the buffer.
> >
> > Seems OK. Do you care about truncation errors? Libraries really should.
>
> Yes, but I think NUL-terminating the strings is more important.
>
> There are several cases here. In cam_get_device(), the user really should
> allocate enough space for the NUL -- if he doesn't, oh well.
>
> As for the instances where we copy strings to/from CCBs, the strings are
> really limited to FOO_IDLEN - 1, due to the way we handle things inside the
> kernel. The device strings inside the kernel are copied around using
> strcpy(), and if a peripheral driver or sim driver name string is longer
> than {DEV,SIM}_IDLEN - 1 characters, strcpy() will overflow the target
> string.
>
> So truncation shouldn't really happen in cases like the one in
> cam_lookup_pass(), because the dev_name can't legitimately be longer than
> DEV_IDLEN - 1 anyway (not including the NUL).
>
> As for the path name in cam_real_open_device(), according to Warner, it
> shouldn't be over MAXPATHLEN characters in any case. As for the
> given_dev_name, as I pointed out above, device names can't legitimately be
> longer than DEV_IDLEN - 1 anyway.
>
> > There are a couple of other strncpy()'s that could use strlcpy(). One
> > already uses explicit NUL termination.
>
> Actually they both do. I converted both to strlcpy(), and also converted an
> strcpy() to strlcpy().
Oops.
> I've attached a new set of diffs, let me know what you think! (They'll go
> through a buildworld test once my cvsup finishes.)
I grepped for LEN and found a couple more minor problems. There are some
off by 1 errors, and you should probably uses sizeof() everywhere except
in the declarations.
- DEV_IDLEN + 1 in cam_open_device() may be correct, but it might be better
spelled using sizeof(). Why is one more byte allocated for the strings
than in the kernel? Does the kernel support non-termInated cam_periph
names?
- DEV_IDLEN in cam_lookup_pass() is correct, but it would be better spelled
using sizeof(). Here we lose if the kernel actually supports non-terminated
cam_periph names. The array size is only DEV_IDLEN in cam_ccb.h, but it
is DEV_IDLEN + 1 in camlib.h and the strlcpy() now prevents any chance
of getting a non-terminated name of maximal length in ccb.cgdl.periph_name.
- MAXPATHLEN+1 in cam_real_open_device() should be MAXPATHLEN.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020426222203.P4255-100000>