BUG: possible NULL pointer dereference in nfs server

[Rick Macklem]

John Baldwin wrote:
> On Tuesday, January 28, 2014 4:40:45 am Roman Divacky wrote:
> > > > Yea, so long as it includes a comment that states this is done
> > > > to
> > > > work around a stupid compiler bug.
> > > 
> > > Ugh.
> > > 
> > > The above has the following bugs:
> > > - gross style bugs (lines longer than 80 characters)
> > > - large code to do nothing
> > > - would be even larger with a comment
> > > - cannot actually work around any compiler bug involving abort(),
> > > since it
> > >    has no effect on production kernels where KASSERT() is null.
> > > 
> > > >> It is present even in your setup :) Just "objdump -d kernel |
> > > >> grep
> > > >> ud2" on kernel compiled
> > > >> by clang.
> > > >>
> > > > I actually use gcc, but I believe you. I'll admit I still don't
> > > > understand
> > > > why having a trap instruction that never gets executed is a bad
> > > > thing?
> > > 
> > > It isn't but trying to link to the noexistent function abort() on
> > > arches that don't have any trap instruction is a bad thing.
> > >  According
> > > the above, this occurs on sparc64.  It must be a gcc bug, since
> > > sparc64
> > > doesn't have clang.  However, I couldn't get gcc on sparc64 to
> > > generate
> > > an abort() for *NULL.  Similarly on x86 (gcc is hard to test on
> > > x86,
> > > since it is broken (not installed) on FreeBSD cluster machines
> > > for
> > 
> > I was testing clang compiled kernel on sparc64.
> > 
> > The problem is that there's nothing making sure that the NULL
> > pointer
> > dereference doesnt happen. So if someone happens to call the
> > function with
> > wrong flags it will crash.
> > 
> > Thats why I want to add the KASSERT, to catch possible future cases
> > when this happens.
> > 
> > Unfortunately our KASSERT is not an assert so to remove the actual
> > abort/trap/ud2 I have to remove the flag.
> 
> Why not make a simple abort() that calls panic()?  It seems clumsy to
> have to
> add hacks in the source code.
> 
> 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?

I think the new_lfpp argument was a vestige
created when nfsrv_getlockfh() became a separate function from nfsrv_getlockfile()
during development. (The latter needs the "**", since it sets it to NULL to
indicate the new structure is being used and shouldn't be free'd.)

rick

> --
> John Baldwin
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
>