From owner-freebsd-bugs@FreeBSD.ORG Mon Jun 28 22:13:48 2010 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DBB48106566B; Mon, 28 Jun 2010 22:13:47 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 75F008FC08; Mon, 28 Jun 2010 22:13:47 +0000 (UTC) Received: from besplex.bde.org (c122-106-145-25.carlnfd1.nsw.optusnet.com.au [122.106.145.25]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o5SMDhda014208 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 29 Jun 2010 08:13:45 +1000 Date: Tue, 29 Jun 2010 08:13:43 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jaakko Heinonen In-Reply-To: <201006281910.o5SJA3xL003167@freefall.freebsd.org> Message-ID: <20100629072734.H2710@besplex.bde.org> References: <201006281910.o5SJA3xL003167@freefall.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-bugs@freebsd.org Subject: Re: kern/144307: ENOENT set unnecessarily under certain circumstances when malloc is called / fails X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2010 22:13:48 -0000 On Mon, 28 Jun 2010, Jaakko Heinonen wrote: > ... > It's a disklabel bug to inspect the errno value in the success case. > POSIX states: "The value of errno should only be examined when it is > ... > This patch for bsdlabel(8) might fix the error message: > > %%% > Index: sbin/bsdlabel/bsdlabel.c > =================================================================== > --- sbin/bsdlabel/bsdlabel.c (revision 209580) > +++ sbin/bsdlabel/bsdlabel.c (working copy) > @@ -312,7 +312,7 @@ static void > fixlabel(struct disklabel *lp) > { > struct partition *dp; > - int i; > + ssize_t i; > > for (i = 0; i < lp->d_npartitions; i++) { > if (i == RAW_PART) > @@ -359,8 +359,11 @@ readboot(void) > fstat(fd, &st); > if (alphacksum && st.st_size <= BBSIZE - 512) { > i = read(fd, bootarea + 512, st.st_size); > - if (i != st.st_size) > + if (i == -1) > err(1, "read error %s", xxboot); > + if (i != st.st_size) > + errx(1, "couldn't read %ju bytes from %s", > + (uintmax_t)st.st_size, xxboot); It's still a bit sloppy: - st_size has type off_t, and we pass it blindly to read() which takes a size_t and actually only accepts sizes up to INT_MAX (not up to SSIZE_MAX as specified). This works due to implicit conversion of the off_t to a size_t, provided no truncation occurs, which is the case unless the file is weird. - short reads are treated as errors - the signed type st_size is printed using the unsigned type uintmax_t. Note that the short read check (i != st.st_size) depends on not having a similar sign mismatch to be correct. I wouldn't change so much here unless I were fixing the short reads. Just use the same error message (including misusing errno when there is no error but a short read) in all cases. But fix the overflow bug by not blindly truncating the result of read by assigning it to i (the SSIZE_MAX bug prevents this assignment overflowing, but depending on this is worth fixing since the fix is complete): if (read(fd, bootarea + 512, st.st_size) != st.st_size) err(1, "read error %s", xxboot); > > /* > * Set the location and length so SRM can find the > @@ -373,8 +376,11 @@ readboot(void) > return; > } else if ((!alphacksum) && st.st_size <= BBSIZE) { > i = read(fd, bootarea, st.st_size); > - if (i != st.st_size) > + if (i == -1) > err(1, "read error %s", xxboot); > + if (i != st.st_size) > + errx(1, "couldn't read %ju bytes from %s", > + (uintmax_t)st.st_size, xxboot); Similarly. > return; > } > errx(1, "boot code %s is wrong size", xxboot); > @@ -499,7 +505,7 @@ readlabel(int flag) > "disks with more than 2^32-1 sectors are not supported"); > (void)lseek(f, (off_t)0, SEEK_SET); > if (read(f, bootarea, BBSIZE) != BBSIZE) > - err(4, "%s read", specname); > + errx(4, "couldn't read %d bytes from %s", BBSIZE, specname); Now the change to using errx() is incomplete, but this is the only case where short reads can actually easily occur in practive (since we read the file size above, while here we read BBSIZE which may exceed the file size for silly devices). Fixing short reads won't help much in the case of a silly device either, since the short read has hit EOF so the next read will just return 0 so that all we can do is tell that the device file is smaller than BBSIZE so it is silly and then print a more specific error message than either of the above. > close (f); > error = bsd_disklabel_le_dec( > bootarea + (labeloffset + labelsoffset * secsize), > %%% Only 1 other use of err() in the program seems to be wrong: after one of g_mediasize() and g_sectorsize() returns < 0. No errno handling is specified for these functions (only for geom_stats_open() in their omnibus man page), so it must be assumed that errno is random after them. Bruce