From owner-freebsd-fs@FreeBSD.ORG Sun Feb 23 21:33:51 2014 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 11ECAF5D; Sun, 23 Feb 2014 21:33:51 +0000 (UTC) Received: from tensor.andric.com (tensor.andric.com [IPv6:2001:7b8:3a7:1:2d0:b7ff:fea0:8c26]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id AFE5C17F8; Sun, 23 Feb 2014 21:33:50 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7::2953:bff3:cb76:c707] (unknown [IPv6:2001:7b8:3a7:0:2953:bff3:cb76:c707]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 3D35B5C45; Sun, 23 Feb 2014 22:33:46 +0100 (CET) Content-Type: multipart/signed; boundary="Apple-Mail=_DEF1A75D-17C6-428B-B203-06F3FD19012B"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 7.1 \(1827\)) Subject: Re: BUG: possible NULL pointer dereference in nfs server From: Dimitry Andric In-Reply-To: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> Date: Sun, 23 Feb 2014 22:33:31 +0100 Message-Id: <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org> References: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> To: Rick Macklem X-Mailer: Apple Mail (2.1827) Cc: freebsd-fs@freebsd.org, Roman Divacky X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Feb 2014 21:33:51 -0000 --Apple-Mail=_DEF1A75D-17C6-428B-B203-06F3FD19012B Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii On 01 Feb 2014, at 00:52, Rick Macklem wrote: > John Baldwin wrote: ... >> Why not make a simple abort() that calls panic()? It seems clumsy to >> have to >> add hacks in the source code. Maybe, but it is better to just avoid undefined behavior. Remember the compiler can do anything it likes here, and it is just being convenient by inserting an "impossible" instruction. However, it might as well insert some of those well-known nasal demons... :) >> OTOH, the new_lfpp thing just seems to be obfuscation. Seems you can >> remove >> one layer of pointer there. It doesn't help you with the compiler >> not being >> able to see the invariant that prevents the problem though. >> >> Index: nfs_nfsdstate.c >> =================================================================== >> --- nfs_nfsdstate.c (revision 261291) >> +++ nfs_nfsdstate.c (working copy) >> @@ -79,7 +79,7 @@ static int nfsrv_getstate(struct nfsclient *clp, n >> static void nfsrv_getowner(struct nfsstatehead *hp, struct nfsstate >> *new_stp, >> struct nfsstate **stpp); >> static int nfsrv_getlockfh(vnode_t vp, u_short flags, >> - struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p); >> + struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p); >> static int nfsrv_getlockfile(u_short flags, struct nfslockfile >> **new_lfpp, >> struct nfslockfile **lfpp, fhandle_t *nfhp, int lockit); >> static void nfsrv_insertlock(struct nfslock *new_lop, >> @@ -1985,7 +1985,7 @@ tryagain: >> MALLOC(new_lfp, struct nfslockfile *, sizeof (struct nfslockfile), >> M_NFSDLOCKFILE, M_WAITOK); >> if (vp) >> - getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp, >> + getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp, >> NULL, p); >> NFSLOCKSTATE(); >> /* >> @@ -2235,7 +2235,7 @@ tryagain: >> M_NFSDSTATE, M_WAITOK); >> MALLOC(new_deleg, struct nfsstate *, sizeof (struct nfsstate), >> M_NFSDSTATE, M_WAITOK); >> - getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp, >> + getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp, >> NULL, p); >> NFSLOCKSTATE(); >> /* >> @@ -3143,10 +3143,9 @@ out: >> */ >> static int >> nfsrv_getlockfh(vnode_t vp, u_short flags, >> - struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p) >> + struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p) >> { >> fhandle_t *fhp = NULL; >> - struct nfslockfile *new_lfp; >> int error; >> >> /* >> @@ -3154,7 +3153,6 @@ nfsrv_getlockfh(vnode_t vp, u_short flags, >> * a fhandle_t on the stack. >> */ >> if (flags & NFSLCK_OPEN) { >> - new_lfp = *new_lfpp; >> fhp = &new_lfp->lf_fh; >> } else if (nfhp) { >> fhp = nfhp; >> >> > Yep, this looks good to me, although I have no idea if it makes the > compiler happy? It seems to, though I think it could still crash if it ever got its flags in an unexpected state, while new_lfp is NULL. Let's just hope that never happens. So can we commit jhb's patch now? That would be very handy for my clang-sparc64 project branch. :) -Dimitry --Apple-Mail=_DEF1A75D-17C6-428B-B203-06F3FD19012B Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) iEYEARECAAYFAlMKaTMACgkQsF6jCi4glqMKqACff44aLvzw4GKlzo1IV2KNb0JD 0/wAoJeegYnHi41LwDrsgQUv4ln7wUKl =SojS -----END PGP SIGNATURE----- --Apple-Mail=_DEF1A75D-17C6-428B-B203-06F3FD19012B--