Date: Mon, 28 Jun 2010 19:50:03 GMT From: Garrett Cooper <gcooper@FreeBSD.org> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/144307: ENOENT set unnecessarily under certain circumstances when malloc is called / fails Message-ID: <201006281950.o5SJo3qW038135@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/144307; it has been noted by GNATS.
From: Garrett Cooper <gcooper@FreeBSD.org>
To: Jaakko Heinonen <jh@freebsd.org>
Cc: bug-followup@freebsd.org
Subject: Re: kern/144307: ENOENT set unnecessarily under certain circumstances
when malloc is called / fails
Date: Mon, 28 Jun 2010 12:46:17 -0700
On Mon, Jun 28, 2010 at 12:05 PM, Jaakko Heinonen <jh@freebsd.org> wrote:
> On 2010-02-26, Garrett Cooper wrote:
>> On systems where /etc/malloc.conf isn't present, some failures
>> syscalls like read will fail incorrectly with ENOENT because the file
>> doesn't exist, and thus output via errx will be incorrect, like shown
>
> I think you mean err(3), not errx(3).
Yeah... true :).
>> from the following disklabel output:
>>
>> 1+0 records in
>> 1+0 records out
>> 512 bytes transferred in 0.000054 secs (9502140 bytes/sec)
>> disklabel: /dev/md1 read: No such file or directory
>
> 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
> indicated to be valid by a function's return value." and "The setting of
> errno after a successful call to a function is unspecified unless the
> description of that function specifies that errno shall not be
> modified.".
>
> This patch for bsdlabel(8) might fix the error message:
>
> %%%
> Index: sbin/bsdlabel/bsdlabel.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- sbin/bsdlabel/bsdlabel.c =A0 =A0(revision 209580)
> +++ sbin/bsdlabel/bsdlabel.c =A0 =A0(working copy)
> @@ -312,7 +312,7 @@ static void
> =A0fixlabel(struct disklabel *lp)
> =A0{
> =A0 =A0 =A0 =A0struct partition *dp;
> - =A0 =A0 =A0 int i;
> + =A0 =A0 =A0 ssize_t i;
Good catch on the implicit cast.
> =A0 =A0 =A0 =A0for (i =3D 0; i < lp->d_npartitions; i++) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (i =3D=3D RAW_PART)
> @@ -359,8 +359,11 @@ readboot(void)
> =A0 =A0 =A0 =A0fstat(fd, &st);
> =A0 =A0 =A0 =A0if (alphacksum && st.st_size <=3D BBSIZE - 512) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0i =3D read(fd, bootarea + 512, st.st_size)=
;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i !=3D st.st_size)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i =3D=3D -1)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0err(1, "read error %s", xx=
boot);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i !=3D st.st_size)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 errx(1, "couldn't read %ju =
bytes from %s",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (uintmax_t)st.st_si=
ze, xxboot);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Set the location and length so SRM can =
find the
> @@ -373,8 +376,11 @@ readboot(void)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
> =A0 =A0 =A0 =A0} else if ((!alphacksum) && st.st_size <=3D BBSIZE) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0i =3D read(fd, bootarea, st.st_size);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i !=3D st.st_size)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i =3D=3D -1)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0err(1, "read error %s", xx=
boot);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i !=3D st.st_size)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 errx(1, "couldn't read %ju =
bytes from %s",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (uintmax_t)st.st_si=
ze, xxboot);
Or the malloc(3) call could be fixed with the couple of lines I
noted (well, adlibbed of course... I wrote the patch before I started
familiarizing myself with style(9))?
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0errx(1, "boot code %s is wrong size", xxboot);
> @@ -499,7 +505,7 @@ readlabel(int flag)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"disks with more than 2^32-1 secto=
rs are not supported");
> =A0 =A0 =A0 =A0(void)lseek(f, (off_t)0, SEEK_SET);
> =A0 =A0 =A0 =A0if (read(f, bootarea, BBSIZE) !=3D BBSIZE)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 err(4, "%s read", specname);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 errx(4, "couldn't read %d bytes from %s", B=
BSIZE, specname);
> =A0 =A0 =A0 =A0close (f);
> =A0 =A0 =A0 =A0error =3D bsd_disklabel_le_dec(
> =A0 =A0 =A0 =A0 =A0 =A0bootarea + (labeloffset + labelsoffset * secsize),
> %%%
Which I agree with, but shouldn't we fix malloc(3) (and any other
function calls that depend on malloc(3) for sensible results)?
Thanks!
-Garrett
PS Neither malloc nor read mentions ENOENT in their respective
manpages (and if they did it would be non-POSIX compliant -
http://www.opengroup.org/onlinepubs/009695399/functions/malloc.html ,
http://opengroup.org/onlinepubs/007908775/xsh/read.html).
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201006281950.o5SJo3qW038135>