From owner-freebsd-i386@FreeBSD.ORG Fri Jan 28 01:40:20 2005 Return-Path: Delivered-To: freebsd-i386@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5A94E16A4CE for ; Fri, 28 Jan 2005 01:40:20 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 22D5243D1F for ; Fri, 28 Jan 2005 01:40:20 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.1/8.13.1) with ESMTP id j0S1eKAa052566 for ; Fri, 28 Jan 2005 01:40:20 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.1/8.13.1/Submit) id j0S1eJSV052565; Fri, 28 Jan 2005 01:40:19 GMT (envelope-from gnats) Date: Fri, 28 Jan 2005 01:40:19 GMT Message-Id: <200501280140.j0S1eJSV052565@freefall.freebsd.org> To: freebsd-i386@FreeBSD.org From: "Mark W. Krentel" Subject: Re: i386/62699: fstat randomly crashes with "out of memory" X-BeenThere: freebsd-i386@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: "Mark W. Krentel" List-Id: I386-specific issues for FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Jan 2005 01:40:20 -0000 The following reply was made to PR i386/62699; it has been noted by GNATS. From: "Mark W. Krentel" To: sigma@pair.com, bug-followup@freebsd.org, freebsd-bugs@freebsd.org Cc: kris@obsecurity.org, alc@cs.rice.edu Subject: Re: i386/62699: fstat randomly crashes with "out of memory" Date: Thu, 27 Jan 2005 20:39:48 -0500 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)