From owner-cvs-all Thu Apr 25 19:19:17 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 AC93B37B41A; Thu, 25 Apr 2002 19:18:55 -0700 (PDT) Received: (from ken@localhost) by panzer.kdm.org (8.11.6/8.9.1) id g3Q2IaM55196; Thu, 25 Apr 2002 20:18:36 -0600 (MDT) (envelope-from ken) Date: Thu, 25 Apr 2002 20:18:36 -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: <20020425201835.A55111@panzer.kdm.org> References: <20020424224122.A48194@panzer.kdm.org> <20020426054140.N1572-100000@gamplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="3MwIy2ne0vdjdPXF" Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: <20020426054140.N1572-100000@gamplex.bde.org>; from bde@zeta.org.au on Fri, Apr 26, 2002 at 05:47:12AM +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 --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Apr 26, 2002 at 05:47:12 +1000, Bruce Evans wrote: > On Wed, 24 Apr 2002, Kenneth D. Merry wrote: > > On Wed, Apr 24, 2002 at 21:46:51 +1000, Bruce Evans wrote: > > > /* > > > * 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'; > > > > > > This used to have an off-by-1 error. Now it has an off-by-2 error. The > > > ... > > > The only problem is, the NUL terminator isn't put in either string, and the > > structure isn't bzeroed, I think. > > > > So what needs to be done is make sure the strncpy() instance above doesn't > > overflow, and make sure the strings are NUL terminated. > > > > 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(). 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.) Thanks, Ken -- Kenneth Merry ken@kdm.org --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="camlib.c.20020425" ==== //depot/FreeBSD-ken/src/lib/libcam/camlib.c#4 - /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c ==== *** /tmp/tmp.26820.0 Thu Apr 25 19:59:38 2002 --- /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c Thu Apr 25 19:52: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); *************** *** 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, DEV_IDLEN); ccb.cgdl.unit_number = unit; /* *************** *** 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'; --- 572,579 ---- * 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; --- 582,589 ---- * 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; --- 635,642 ---- } 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; --- 647,653 ---- "%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; --3MwIy2ne0vdjdPXF-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message