Date: Tue, 29 Jun 2010 08:13:43 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@freebsd.org> Cc: freebsd-bugs@freebsd.org Subject: Re: kern/144307: ENOENT set unnecessarily under certain circumstances when malloc is called / fails Message-ID: <20100629072734.H2710@besplex.bde.org> In-Reply-To: <201006281910.o5SJA3xL003167@freefall.freebsd.org> References: <201006281910.o5SJA3xL003167@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100629072734.H2710>