BUG: possible NULL pointer dereference in nfs server

[Dimitry Andric]

On 01 Feb 2014, at 00:52, Rick Macklem <rmacklem at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 203 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20140223/38912ad9/attachment.sig>