Date: Sun, 23 Feb 2014 22:33:31 +0100 From: Dimitry Andric <dim@FreeBSD.org> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: freebsd-fs@freebsd.org, Roman Divacky <rdivacky@freebsd.org> Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <3B667D71-2ED6-4EBF-A586-297BEDC9499A@FreeBSD.org> In-Reply-To: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> References: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <rmacklem@uoguelph.ca> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3B667D71-2ED6-4EBF-A586-297BEDC9499A>