From: "J. Bruce Fields" <bfields@fieldses.org>
Subject: Re: [PATCH] Fix incorrect assignment
Date: Thu, 25 Oct 2007 19:12:34 -0400
Message-ID: <20071025231234.GL31888@fieldses.org>
References: <20071024205736.GL29830@fieldses.org>
	<OF5B1D195B.8B37E3EE-ON8725737F.00547AB2-8625737F.0054CCF1@us.ibm.com>
	<20071025160319.GA31888@fieldses.org> <4720D93A.1000300@us.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: Prasad P <pvp@us.ibm.com>, nfs@lists.sourceforge.net
To: Steve French <smfltc@us.ibm.com>
In-Reply-To: <4720D93A.1000300@us.ibm.com>
Sender: nfs-bounces@lists.sourceforge.net
Errors-To: nfs-bounces@lists.sourceforge.net

On Thu, Oct 25, 2007 at 12:58:18PM -0500, Steve French wrote:
> J. Bruce Fields wrote:
> > On Thu, Oct 25, 2007 at 10:26:14AM -0500, Prasad P wrote:
> >   
> >> nfs-bounces@lists.sourceforge.net wrote on 10/24/2007 03:57:36 PM:
> >>     
> >>> On Wed, Oct 24, 2007 at 03:14:32PM -0500, Prasad P wrote:
> >>>       
> >>>> Dereferenced pointer "dentry" without checking and assigned to inode
> >>>> in the declaration.
> >>>>         
> >>> Seems reasonable, but: looking at nfsd_dispatch(), it appears that the
> >>> encode function is never called in this case (since rq_vers == 2 and
> >>> nfsacld_proc_getacl() would have returned an error if it couldn't find a
> >>> dentry).  Am I missing something?  Do you have a test case?
> >>>
> >>> --b.
> >>>       
> >> No, I don't have a test case.  This error was found by coverity scan.
> >>
> >> After declaration of the variables, we actually check the dentry and
> >> dentry->d_inode, and if they are not NULL, assign dentry->d_inode to inode
> >> variable.
> >>
> >> if (dentry == NULL || dentry->d_inode == NULL)
> >>       return 0;
> >> inode = dentry->d_inode;
> >>     
> >
> > Yeah, the current code is obviously a little schizophrenic.  I'm just
> > wondering whether we should fix it by deleting the early assignment or
> > by removing the unnecessary checks.
> >   
> 
> My preference would be the more intuitive and also safer answer (in the 
> long term) ie to remove the early assignment.  It saves having to think 
> as much in the long term about error conditions in the callers of this 
> function.

My inclination is usually to fail obviously if we've got to fail, on the
theory it makes it easier to catch bugs.  All the more so when it'd give
me an excuse to delete a couple more lines of code.

But I agree that verifying that this function is never called in the bad
case takes a little too much investigation.  So, fair enough; applied.
Thanks for the patch.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs