i386/62699: fstat randomly crashes with "out of memory"

[Mark W. Krentel]

The analysis by the originator, Kevin Martin, is correct.  I looked
into this and indeed, filed.fd_lastfile does occasionally have a way-
out-of-bounds value (positive or negative), leading to an "out of
memory" crash, or worse.

This is the same bug reported by Kris Kennaway in the thread "fstat
triggered INVARIANTS panic in memrw()" on -current in January 2005.
Except that Kris was getting a panic because kernacc() in vm_glue.c
was not checking for address wrap.  That has now been patched.

This is a classic case of the transitory nature of the data obtained
from kvm_read() in a running kernel.  Even if kvm_read() returns
successfully, there is no guarantee that the data is meaningful or
what you expect.  It may be garbage, and even if it's valid, it may be
out of date by the time you see it.

There is no exact "fix", the best you can do is a sanity check on
filed.fd_lastfile, as you suggest.  One could argue over the best
bound, I went with 0x01000000 in the patch below.  In my tests, all
of the out-of-bounds values were either negative or greater than
0x70000000, so I decided that 0x01000000 (16 million) was a reasonable
compromise for the number of file descriptors that are simultaneously
open.

Note that fd_lastfile == -1 is valid, it just means that the process
has no open files.  In this case, there's no reason to continue with
the "open files" section of dofiles().

P.S. This PR should be category "bin", not "i386".  The problem is in
fstat(1), there's nothing x86-specific about it.

P.P.S. Please limit lines to about 75-80 characters.

--Mark

Patch for the "out of memory" problem:

Index: fstat.c
===================================================================
RCS file: /data/ncvs/src/usr.bin/fstat/fstat.c,v
retrieving revision 1.57
diff -u -r1.57 fstat.c
--- fstat.c	11 Jan 2005 18:52:12 -0000	1.57
+++ fstat.c	27 Jan 2005 22:31:28 -0000
@@ -358,6 +358,12 @@
 	 * open files
 	 */
 #define FPSIZE	(sizeof (struct file *))
+#define	MAX_LASTFILE  (0x01000000)
+
+	/* Sanity check on filed.fd_lastfile */
+	if (filed.fd_lastfile <= -1 || filed.fd_lastfile > MAX_LASTFILE)
+		return;
+
 	ALLOC_OFILES(filed.fd_lastfile+1);
 	if (!KVM_READ(filed.fd_ofiles, ofiles,
 	    (filed.fd_lastfile+1) * FPSIZE)) {


Two other points, while I'm here.  First, in the macro definition of
KVM_READ in fstat.h, the check for "(len) < SSIZE_MAX" is pretty much
pointless.  What positive value for len is ever greater than SSIZE_MAX?
It would make more sense to check for "(len) > 0" because if len == 0
then kvm_read() would return 0 bytes and KVM_READ would return true
but not return any useful data.

Actually, since all but one use of KVM_READ has sizeof(...) for len,
and the last use is in the patch above, I would skip the test
altogether and write it as:

#define	KVM_READ(kaddr, paddr, len) \
	(kvm_read(kd, (u_long)(kaddr), (char *)(paddr), (len)) == (ssize_t)(len))


Second, dofiles() should also include the jail root directory, if
there is one.  I'm guessing that fstat(1) was written long before
there were jails.

Index: fstat.c
===================================================================
RCS file: /data/ncvs/src/usr.bin/fstat/fstat.c,v
retrieving revision 1.57
diff -u -r1.57 fstat.c
--- fstat.c	11 Jan 2005 18:52:12 -0000	1.57
+++ fstat.c	28 Jan 2005 01:02:21 -0000
@@ -106,6 +106,7 @@
 #define	RDIR	-3
 #define	TRACE	-4
 #define	MMAP	-5
+#define	JDIR	-6
 
 DEVS *devs;
 
@@ -309,6 +310,9 @@
 	case MMAP: \
 		printf(" mmap"); \
 		break; \
+	case JDIR: \
+		printf(" jail"); \
+		break; \
 	default: \
 		printf(" %4d", i); \
 		break; \
@@ -345,6 +349,11 @@
 	 */
 	vtrans(filed.fd_cdir, CDIR, FREAD);
 	/*
+	 * Jail root directory vnode.
+	 */
+	if (filed.fd_jdir != NULL)
+		vtrans(filed.fd_jdir, JDIR, FREAD);
+	/*
 	 * ktrace vnode, if one
 	 */
 	if (kp->ki_tracep)