From owner-cvs-all Wed May 1 19:57: 6 2002 Delivered-To: cvs-all@freebsd.org Received: from panzer.kdm.org (panzer.kdm.org [216.160.178.169]) by hub.freebsd.org (Postfix) with ESMTP id ECCC137B405; Wed, 1 May 2002 19:56:48 -0700 (PDT) Received: (from ken@localhost) by panzer.kdm.org (8.11.6/8.9.1) id g422uMr01524; Wed, 1 May 2002 20:56:22 -0600 (MDT) (envelope-from ken) Date: Wed, 1 May 2002 20:56:22 -0600 From: "Kenneth D. Merry" To: Bruce Evans Cc: "M. Warner Losh" , 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> References: <20020425201835.A55111@panzer.kdm.org> <20020426222203.P4255-100000@gamplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="M9NhX3UHpAaciwkO" Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: <20020426222203.P4255-100000@gamplex.bde.org>; from bde@zeta.org.au on Fri, Apr 26, 2002 at 10:45:06PM +1000 Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG --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