cvs commit: src/sys/nfsserver nfs_serv.c

[John Baldwin]

On Monday 23 January 2006 15:06, Tom Rhodes wrote:
> On Mon, 23 Jan 2006 14:59:22 -0500
>
> John Baldwin <jhb at freebsd.org> wrote:
> > On Monday 23 January 2006 14:28, Tom Rhodes wrote:
> > > On Mon, 23 Jan 2006 13:56:16 -0500
> > >
> > > John Baldwin <jhb at FreeBSD.org> wrote:
> > > > On Saturday 21 January 2006 07:10, Tom Rhodes wrote:
> > > > > trhodes     2006-01-21 12:10:33 UTC
> > > > >
> > > > >   FreeBSD src repository
> > > > >
> > > > >   Modified files:
> > > > >     sys/nfsserver        nfs_serv.c
> > > > >   Log:
> > > > >   Remove some dead code.
> > > > >
> > > > >   Found with:     Coverity Prevent(tm)
> > > >
> > > > Are you going to revert this change given the replies?
> > >
> > > Oh, I didn't interpret the comments as "this is wrong please
> > > back it out."  I just seen replies, both public and private,
> > > complaining about the indentation.  They went like:
> > >
> > > stefanf: "Are you sure this is correct?"
> >
> > When someone says this, you generally should be able to reply with either
> > "Yes, because of X, Y, and Z", or "oops, I guess not, I'll back it out"
>
> I did reply, but forgot to CC: the cvs lists.  There was just
> no bothering to follow up since I figured the discussion died,
> since stefan never followed up.
>
> I'll find the reply and push it off.
>
> > >   rwatson: "code is a mess in NFS"
> > >
> > > ru: quoting the code "bad indentation"
> > > njl quoting the code "bad indentation"
> > >
> > > rees (NFSv4 guy): "looks fine to me"
> > >
> > > If you, or anyone else for that matter actually wants it
> > > reverted, I'll do that.  I'm not in the mood to argue
> > > with people today, or ever.  :)
> >
> > <quote from="stefanf">
> > Hm, are you sure this change is correct?  Apparently Coverity thinks
> > that dirp is always 0 at this point, yes?  Looking at nfs_namei() I
> > don't believe that.
> > </quote>
> >
> > Note the "I don't believe that" part.
> >
> > <quote from="Peter Jeremy">
> > I'll put my $0.02 in and agree with Stefan Farfeleder.  (Luckily, in
> > this case, the notorious NFS macros are not involved).  The comments
> > on nfs_namei() state that dirp can be returned not-NULL even if an
> > error occurs and a check of the code paths in nfs_namei() indicates
> > that this is correct.  Can you please re-evaluate your change.
> >
> > If (as I suspect), this is actually an incorrect report from Coverity,
> > we should probably report it back to them to investigate.
> > </quote>
> >
> > Please either offer explanations to address the concerns raised or back
> > it out.
>
> ---------------------------------------------------------------
>
> > Hm, are you sure this change is correct?  Apparently Coverity thinks
> > that dirp is always 0 at this point, yes?  Looking at nfs_namei() I
> > don't believe that.  Also the comment above this is now stale and the
> > code inside 'if (error)' not indented properly.
>
> Yes, this change should be correct.  dirp is always 0 except for
> one part (which you mention above) and is used/tested elsewhere
> for that reason.  njl and ru have already got me on the stale
> comment and indention.  Jim Reese (NFSv4 guy) also feels that
> this commit is "good."  So I got post-commit review.  ;)
>
> But I'll definitely agree with rwatson, the nfs code is really
> scary.  :)
> --------------------------------------------------------------

Hmm, what do you mean by "one part"?  For example, in nfs_namei() you can have 
dirp != NULL in the following error cases (look for BEWM):

int
nfs_namei(struct nameidata *ndp, fhandle_t *fhp, int len,
    struct nfssvc_sock *slp, struct sockaddr *nam, struct mbuf **mdp,
    caddr_t *dposp, struct vnode **retdirp, int v3, struct vattr *retdirattrp,
    int *retdirattr_retp, struct thread *td, int pubflag)
{
	...

	*retdirp = NULL;

	...

	/*
	 * Set return directory.  Reference to dp is implicitly transfered
	 * to the returned pointer
	 */
	*retdirp = dp;

	...
	if (pubflag) {
		...
		if ((unsigned char)*fromcp >= WEBNFS_SPECCHAR_START) {
			switch ((unsigned char)*fromcp) {
			...
			default:
				error = EIO;
				uma_zfree(namei_zone, cp);
				goto out;     <=== BEWM!!!!
			}
		}
		...
		while (*fromcp != '\0') {
			if (*fromcp == WEBNFS_ESC_CHAR) {
				if (fromcp[1] != '\0' && fromcp[2] != '\0') {
					...
				} else {
					error = ENOENT;
					uma_zfree(namei_zone, cp);
					goto out;  <=== BEWM!!!!
				}
			...
		}
	...
	for (;;) {
		...
		error = lookup(ndp);
		if (error)
			break;  <=== BEWM!!!
		...
		error = VOP_READLINK(ndp->ni_vp, &auio, cnp->cn_cred);
		if (error) {
			...
			vrele(ndp->ni_dvp);
			vput(ndp->ni_vp);
			break;  <<< BEWM!!!!
		}
		...
	}
	...
out:
	...
}

Won't your changes now be leaking a vnode reference in those cases?

-- 
John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org