From owner-cvs-all Fri Apr 26 5:46:50 2002 Delivered-To: cvs-all@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id 9B95737B419; Fri, 26 Apr 2002 05:46:39 -0700 (PDT) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id WAA16385; Fri, 26 Apr 2002 22:43:59 +1000 Date: Fri, 26 Apr 2002 22:45:06 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: "Kenneth D. Merry" Cc: "M. Warner Losh" , , , Subject: Re: cvs commit: src/lib/libcam camlib.h In-Reply-To: <20020425201835.A55111@panzer.kdm.org> Message-ID: <20020426222203.P4255-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 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