Date: Wed, 1 May 2002 20:56:22 -0600 From: "Kenneth D. Merry" <ken@kdm.org> To: Bruce Evans <bde@zeta.org.au> 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: <20020501205622.A1494@panzer.kdm.org> In-Reply-To: <20020426222203.P4255-100000@gamplex.bde.org>; from bde@zeta.org.au on Fri, Apr 26, 2002 at 10:45:06PM %2B1000 References: <20020425201835.A55111@panzer.kdm.org> <20020426222203.P4255-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--M9NhX3UHpAaciwkO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Fri, Apr 26, 2002 at 22:45:06 +1000, Bruce Evans wrote:
> 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?
Fixed. The original reason for making the strings N+1 was probably to
support unterminated strings coming from the kernel.
As a practical matter, I don't think unterminated strings are a good idea.
I'll talk it over with Justin and we'll come to some consensus on it.
> - 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.
Fixed. I kinda doubt we really support unterminated strings anyway.
> - MAXPATHLEN+1 in cam_real_open_device() should be MAXPATHLEN.
It wasn't used, so I deleted the declaration.
Ken
--
Kenneth Merry
ken@kdm.org
--M9NhX3UHpAaciwkO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="camlib.c.20020501"
==== //depot/FreeBSD-ken/src/lib/libcam/camlib.c#4 - /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c ====
*** /tmp/tmp.1133.0 Wed May 1 20:48:20 2002
--- /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c Sat Apr 27 22:01:48 2002
***************
*** 289,304 ****
*/
for (i = 0;i < (sizeof(devmatchtable)/sizeof(struct cam_devequiv));i++){
if (strcmp(tmpstr, devmatchtable[i].given_dev) == 0) {
! strncpy(dev_name,devmatchtable[i].real_dev, devnamelen);
found = 1;
break;
}
}
if (found == 0)
! strncpy(dev_name, tmpstr, devnamelen);
!
! /* Make sure we pass back a null-terminated string */
! dev_name[devnamelen - 1] = '\0';
/* Clean up allocated memory */
free(newpath);
--- 289,301 ----
*/
for (i = 0;i < (sizeof(devmatchtable)/sizeof(struct cam_devequiv));i++){
if (strcmp(tmpstr, devmatchtable[i].given_dev) == 0) {
! strlcpy(dev_name,devmatchtable[i].real_dev, devnamelen);
found = 1;
break;
}
}
if (found == 0)
! strlcpy(dev_name, tmpstr, devnamelen);
/* Clean up allocated memory */
free(newpath);
***************
*** 322,328 ****
* cam_get_device() has already put an error message in cam_errbuf,
* so we don't need to.
*/
! if (cam_get_device(path, dev_name, DEV_IDLEN + 1, &unit) == -1)
return(NULL);
return(cam_lookup_pass(dev_name, unit, flags, path, NULL));
--- 319,325 ----
* cam_get_device() has already put an error message in cam_errbuf,
* so we don't need to.
*/
! if (cam_get_device(path, dev_name, sizeof(dev_name), &unit) == -1)
return(NULL);
return(cam_lookup_pass(dev_name, unit, flags, path, NULL));
***************
*** 490,497 ****
ccb.ccb_h.func_code = XPT_GDEVLIST;
/* These two are necessary for the GETPASSTHRU ioctl to work. */
! strncpy(ccb.cgdl.periph_name, dev_name, DEV_IDLEN - 1);
! ccb.cgdl.periph_name[DEV_IDLEN - 1] = '\0';
ccb.cgdl.unit_number = unit;
/*
--- 487,493 ----
ccb.ccb_h.func_code = XPT_GDEVLIST;
/* These two are necessary for the GETPASSTHRU ioctl to work. */
! strlcpy(ccb.cgdl.periph_name, dev_name, sizeof(ccb.cgdl.periph_name));
ccb.cgdl.unit_number = unit;
/*
***************
*** 552,558 ****
const char *given_path, const char *given_dev_name,
int given_unit_number)
{
- char newpath[MAXPATHLEN+1];
char *func_name = "cam_real_open_device";
union ccb ccb;
int fd = -1, malloced_device = 0;
--- 548,553 ----
***************
*** 576,582 ****
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
! strncpy(device->device_path, given_path, MAXPATHLEN + 1);
else
device->device_path[0] = '\0';
--- 571,578 ----
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
! strlcpy(device->device_path, given_path,
! sizeof(device->device_path));
else
device->device_path[0] = '\0';
***************
*** 585,591 ****
* those as well.
*/
if (given_dev_name != NULL)
! strncpy(device->given_dev_name, given_dev_name, DEV_IDLEN);
else
device->given_dev_name[0] = '\0';
device->given_unit_number = given_unit_number;
--- 581,588 ----
* those as well.
*/
if (given_dev_name != NULL)
! strlcpy(device->given_dev_name, given_dev_name,
! sizeof(device->given_dev_name));
else
device->given_dev_name[0] = '\0';
device->given_unit_number = given_unit_number;
***************
*** 637,643 ****
}
device->dev_unit_num = ccb.cgdl.unit_number;
! strcpy(device->device_name, ccb.cgdl.periph_name);
device->path_id = ccb.ccb_h.path_id;
device->target_id = ccb.ccb_h.target_id;
device->target_lun = ccb.ccb_h.target_lun;
--- 634,641 ----
}
device->dev_unit_num = ccb.cgdl.unit_number;
! strlcpy(device->device_name, ccb.cgdl.periph_name,
! sizeof(device->device_name));
device->path_id = ccb.ccb_h.path_id;
device->target_id = ccb.ccb_h.target_id;
device->target_lun = ccb.ccb_h.target_lun;
***************
*** 648,654 ****
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strncpy(device->sim_name, ccb.cpi.dev_name, SIM_IDLEN);
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
--- 646,652 ----
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strlcpy(device->sim_name, ccb.cpi.dev_name, sizeof(device->sim_name));
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
--M9NhX3UHpAaciwkO--
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?20020501205622.A1494>