kern/144307: ENOENT set unnecessarily under certain circumstances when malloc is called / fails

[Bruce Evans]

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