Bug 144307 - [libc] [patch] ENOENT set unnecessarily under certain circumstances when malloc is called / fails
Summary: [libc] [patch] ENOENT set unnecessarily under certain circumstances when mall...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Jaakko Heinonen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-26 01:20 UTC by Enji Cooper
Modified: 2013-02-03 22:28 UTC (History)
0 users

See Also:


Attachments
file.diff (664 bytes, patch)
2010-02-26 01:20 UTC, Enji Cooper
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer 2010-02-26 01:20:01 UTC
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 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

This can be reproduced as follows (yes, the dd command are stat commands are wrong -- I know that...) if malloc.conf is not present:

dd if=/dev/zero of=$output_file conv=sparse bs=$(stat -f '%z' "$input_directory") count=1
mdconfig -a -t vnode -u ${md_num} -f "$output_file"
disklabel -rw /dev/md${md_num} auto

Once malloc.conf is present...

sudo ln -s M /etc/malloc.conf

# ...

1+0 records in
1+0 records out
512 bytes transferred in 0.000054 secs (9460280 bytes/sec)
disklabel: /dev/md1 read: Unknown error: 0

Fix: 1. Set malloc.conf to a valid value as per malloc(3).
2. Apply attached patch.

Patch attached with submission follows:
How-To-Repeat: dd if=/dev/zero of=foo conv=sparse bs=512 count=1
mdconfig -a -t vnode -u 1 -f foo
disklabel -rw /dev/md1 auto
Comment 1 Jaakko Heinonen freebsd_committer 2010-06-28 20:05:40 UTC
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).

> 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
===================================================================
--- 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);
 
 		/*
 		 * 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);
 		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);
 	close (f);
 	error = bsd_disklabel_le_dec(
 	    bootarea + (labeloffset + labelsoffset * secsize),
%%%

-- 
Jaakko
Comment 2 Enji Cooper freebsd_committer 2010-06-28 20:46:17 UTC
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).
Comment 3 Jaakko Heinonen freebsd_committer 2010-06-28 21:10:11 UTC
On 2010-06-28, Garrett Cooper wrote:
>     Or the malloc(3) call could be fixed with the couple of lines I
> noted (well, adlibbed of course...
> 
>     Which I agree with, but shouldn't we fix malloc(3) (and any other
> function calls that depend on malloc(3) for sensible results)?

It's not required for POSIX compliance at least. Did you actually read
the quotes from POSIX?

"The value of errno should only be examined when it is indicated to be
valid by a function's return value."

"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."

In other words the value of errno is undefined and shouldn't be
examined unless malloc(3) returns NULL.

-- 
Jaakko
Comment 4 Enji Cooper freebsd_committer 2010-06-28 23:17:32 UTC
On Mon, Jun 28, 2010 at 1:10 PM, Jaakko Heinonen <jh@freebsd.org> wrote:
> On 2010-06-28, Garrett Cooper wrote:
>> =A0 =A0 Or the malloc(3) call could be fixed with the couple of lines I
>> noted (well, adlibbed of course...
>>
>> =A0 =A0 Which I agree with, but shouldn't we fix malloc(3) (and any othe=
r
>> function calls that depend on malloc(3) for sensible results)?
>
> It's not required for POSIX compliance at least. Did you actually read
> the quotes from POSIX?
>
> "The value of errno should only be examined when it is indicated to be
> valid by a function's return value."
>
> "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."
>
> In other words the value of errno is undefined and shouldn't be
> examined unless malloc(3) returns NULL.

    Ok. The bsdlabel(8) item is valid, but in the meantime I'll write
some testcases for read(2) to see whether or not I can find the
recreate the failing condition when =3D=3D -1. Worst case we'll have more
testcases to put in the tree...
Thanks!
-Garrett
Comment 5 Jaakko Heinonen freebsd_committer 2010-06-30 19:10:48 UTC
Responsible Changed
From-To: freebsd-bugs->jh

Take.
Comment 6 dfilter service freebsd_committer 2010-06-30 19:35:00 UTC
Author: jh
Date: Wed Jun 30 18:34:45 2010
New Revision: 209614
URL: http://svn.freebsd.org/changeset/base/209614

Log:
  - Don't assign the return value from read(2) to a variable of type
    int.
  - Use errx(3) instead of err(3) to print the error message on short
    reads in readlabel(). errno won't be set on short reads which can
    easily occur here due to the fixed size read request.
  
  PR:		144307
  Reviewed by:	bde

Modified:
  head/sbin/bsdlabel/bsdlabel.c

Modified: head/sbin/bsdlabel/bsdlabel.c
==============================================================================
--- head/sbin/bsdlabel/bsdlabel.c	Wed Jun 30 18:03:42 2010	(r209613)
+++ head/sbin/bsdlabel/bsdlabel.c	Wed Jun 30 18:34:45 2010	(r209614)
@@ -347,7 +347,7 @@ makelabel(const char *type, struct diskl
 static void
 readboot(void)
 {
-	int fd, i;
+	int fd;
 	struct stat st;
 	uint64_t *p;
 
@@ -358,8 +358,7 @@ readboot(void)
 		err(1, "cannot open %s", xxboot);
 	fstat(fd, &st);
 	if (alphacksum && st.st_size <= BBSIZE - 512) {
-		i = read(fd, bootarea + 512, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea + 512, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 
 		/*
@@ -372,8 +371,7 @@ readboot(void)
 		p[62] = 0;
 		return;
 	} else if ((!alphacksum) && st.st_size <= BBSIZE) {
-		i = read(fd, bootarea, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 		return;
 	}
@@ -479,6 +477,7 @@ get_file_parms(int f)
 static int
 readlabel(int flag)
 {
+	ssize_t nbytes;
 	uint32_t lba;
 	int f, i;
 	int error;
@@ -498,8 +497,11 @@ readlabel(int flag)
 		errx(1,
 		    "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)
+	nbytes = read(f, bootarea, BBSIZE);
+	if (nbytes == -1)
 		err(4, "%s read", specname);
+	if (nbytes != BBSIZE)
+		errx(4, "couldn't read %d bytes from %s", BBSIZE, specname);
 	close (f);
 	error = bsd_disklabel_le_dec(
 	    bootarea + (labeloffset + labelsoffset * secsize),
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 7 Jaakko Heinonen freebsd_committer 2010-06-30 19:53:31 UTC
State Changed
From-To: open->patched

bsdlabel(8) patched in head (r209614).
Comment 8 dfilter service freebsd_committer 2010-08-02 10:03:45 UTC
Author: jh
Date: Mon Aug  2 09:03:31 2010
New Revision: 210744
URL: http://svn.freebsd.org/changeset/base/210744

Log:
  MFC r209614:
  
  - Don't assign the return value from read(2) to a variable of type
    int.
  - Use errx(3) instead of err(3) to print the error message on short
    reads in readlabel(). errno won't be set on short reads which can
    easily occur here due to the fixed size read request.
  
  PR:		144307

Modified:
  stable/8/sbin/bsdlabel/bsdlabel.c
Directory Properties:
  stable/8/sbin/bsdlabel/   (props changed)

Modified: stable/8/sbin/bsdlabel/bsdlabel.c
==============================================================================
--- stable/8/sbin/bsdlabel/bsdlabel.c	Mon Aug  2 08:35:16 2010	(r210743)
+++ stable/8/sbin/bsdlabel/bsdlabel.c	Mon Aug  2 09:03:31 2010	(r210744)
@@ -347,7 +347,7 @@ makelabel(const char *type, struct diskl
 static void
 readboot(void)
 {
-	int fd, i;
+	int fd;
 	struct stat st;
 	uint64_t *p;
 
@@ -358,8 +358,7 @@ readboot(void)
 		err(1, "cannot open %s", xxboot);
 	fstat(fd, &st);
 	if (alphacksum && st.st_size <= BBSIZE - 512) {
-		i = read(fd, bootarea + 512, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea + 512, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 
 		/*
@@ -372,8 +371,7 @@ readboot(void)
 		p[62] = 0;
 		return;
 	} else if ((!alphacksum) && st.st_size <= BBSIZE) {
-		i = read(fd, bootarea, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 		return;
 	}
@@ -479,6 +477,7 @@ get_file_parms(int f)
 static int
 readlabel(int flag)
 {
+	ssize_t nbytes;
 	uint32_t lba;
 	int f, i;
 	int error;
@@ -498,8 +497,11 @@ readlabel(int flag)
 		errx(1,
 		    "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)
+	nbytes = read(f, bootarea, BBSIZE);
+	if (nbytes == -1)
 		err(4, "%s read", specname);
+	if (nbytes != BBSIZE)
+		errx(4, "couldn't read %d bytes from %s", BBSIZE, specname);
 	close (f);
 	error = bsd_disklabel_le_dec(
 	    bootarea + (labeloffset + labelsoffset * secsize),
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 9 dfilter service freebsd_committer 2010-08-18 15:41:44 UTC
Author: jh
Date: Wed Aug 18 14:41:34 2010
New Revision: 211454
URL: http://svn.freebsd.org/changeset/base/211454

Log:
  MFC r209614:
  
  - Don't assign the return value from read(2) to a variable of type
    int.
  - Use errx(3) instead of err(3) to print the error message on short
    reads in readlabel(). errno won't be set on short reads which can
    easily occur here due to the fixed size read request.
  
  PR:		144307

Modified:
  stable/7/sbin/bsdlabel/bsdlabel.c
Directory Properties:
  stable/7/sbin/bsdlabel/   (props changed)

Modified: stable/7/sbin/bsdlabel/bsdlabel.c
==============================================================================
--- stable/7/sbin/bsdlabel/bsdlabel.c	Wed Aug 18 12:52:21 2010	(r211453)
+++ stable/7/sbin/bsdlabel/bsdlabel.c	Wed Aug 18 14:41:34 2010	(r211454)
@@ -334,7 +334,7 @@ makelabel(const char *type, struct diskl
 static void
 readboot(void)
 {
-	int fd, i;
+	int fd;
 	struct stat st;
 	uint64_t *p;
 
@@ -345,8 +345,7 @@ readboot(void)
 		err(1, "cannot open %s", xxboot);
 	fstat(fd, &st);
 	if (alphacksum && st.st_size <= BBSIZE - 512) {
-		i = read(fd, bootarea + 512, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea + 512, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 
 		/*
@@ -359,8 +358,7 @@ readboot(void)
 		p[62] = 0;
 		return;
 	} else if ((!alphacksum) && st.st_size <= BBSIZE) {
-		i = read(fd, bootarea, st.st_size);
-		if (i != st.st_size)
+		if (read(fd, bootarea, st.st_size) != st.st_size)
 			err(1, "read error %s", xxboot);
 		return;
 	}
@@ -466,6 +464,7 @@ get_file_parms(int f)
 static int
 readlabel(int flag)
 {
+	ssize_t nbytes;
 	int f, i;
 	int error;
 	struct gctl_req *grq;
@@ -484,8 +483,11 @@ readlabel(int flag)
 		errx(1,
 		    "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)
+	nbytes = read(f, bootarea, BBSIZE);
+	if (nbytes == -1)
 		err(4, "%s read", specname);
+	if (nbytes != BBSIZE)
+		errx(4, "couldn't read %d bytes from %s", BBSIZE, specname);
 	close (f);
 	error = bsd_disklabel_le_dec(
 	    bootarea + (labeloffset + labelsoffset * secsize),
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 10 Jaakko Heinonen freebsd_committer 2010-08-18 15:55:49 UTC
State Changed
From-To: patched->closed

bsdlabel(8) fixed in head, stable/8 and stable/7. I don't consider the 
malloc behavior as a bug but if you still want to pursue the malloc 
change, I suggest trying to contact jasone@ directly. Thanks!