Dereferenced pointer "dentry" without checking and assigned to inode
in the declaration.
Cc: Steve French <[email protected]>
Signed-off-by: Prasad V Potluri <[email protected]>
---
diff -uprN a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
--- a/fs/nfsd/nfs2acl.c 2007-10-09 15:31:38.000000000 -0500
+++ b/fs/nfsd/nfs2acl.c 2007-10-24 14:03:13.000000000 -0500
@@ -221,7 +221,7 @@ static int nfsaclsvc_encode_getaclres(st
struct nfsd3_getaclres *resp)
{
struct dentry *dentry = resp->fh.fh_dentry;
- struct inode *inode = dentry->d_inode;
+ struct inode *inode;
struct kvec *head = rqstp->rq_res.head;
unsigned int base;
int n;
--
Thanks.
Regards,
Prasad Potluri
email: pvp at-sign us dot ibm dot com
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.
>
> Cc: Steve French <[email protected]>
> Signed-off-by: Prasad V Potluri <[email protected]>
> ---
>
> diff -uprN a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> --- a/fs/nfsd/nfs2acl.c 2007-10-09 15:31:38.000000000 -0500
> +++ b/fs/nfsd/nfs2acl.c 2007-10-24 14:03:13.000000000 -0500
> @@ -221,7 +221,7 @@ static int nfsaclsvc_encode_getaclres(st
> struct nfsd3_getaclres *resp)
> {
> struct dentry *dentry = resp->fh.fh_dentry;
> - struct inode *inode = dentry->d_inode;
> + struct inode *inode;
> struct kvec *head = rqstp->rq_res.head;
> unsigned int base;
> int n;
> --
> Thanks.
>
> Regards,
> Prasad Potluri
> email: pvp at-sign us dot ibm dot com
> -------------------------------------------------------------------------
> 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 - [email protected]
> https://lists.sourceforge.net/lists/listinfo/nfs
-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
[email protected] 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;
>
>
> >
> > Cc: Steve French <[email protected]>
> > Signed-off-by: Prasad V Potluri <[email protected]>
> > ---
> >
> > diff -uprN a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> > --- a/fs/nfsd/nfs2acl.c 2007-10-09 15:31:38.000000000 -0500
> > +++ b/fs/nfsd/nfs2acl.c 2007-10-24 14:03:13.000000000 -0500
> > @@ -221,7 +221,7 @@ static int nfsaclsvc_encode_getaclres(st
> > struct nfsd3_getaclres *resp)
> > {
> > struct dentry *dentry = resp->fh.fh_dentry;
> > - struct inode *inode = dentry->d_inode;
> > + struct inode *inode;
> > struct kvec *head = rqstp->rq_res.head;
> > unsigned int base;
> > int n;
> > --
> > Thanks.
> >
> > Regards,
> > Prasad Potluri
> > email: pvp at-sign us dot ibm dot com
> >
-------------------------------------------------------------------------
> > 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 - [email protected]
> > https://lists.sourceforge.net/lists/listinfo/nfs
>
>
> -------------------------------------------------------------------------
> 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 - [email protected]
> https://lists.sourceforge.net/lists/listinfo/nfs
Thanks.
Regards,
Prasad Potluri
email: pvp at-sign us dot ibm dot com
On Thu, Oct 25, 2007 at 10:26:14AM -0500, Prasad P wrote:
> [email protected] 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.
--b.
>
> >
> >
> > >
> > > Cc: Steve French <[email protected]>
> > > Signed-off-by: Prasad V Potluri <[email protected]>
> > > ---
> > >
> > > diff -uprN a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> > > --- a/fs/nfsd/nfs2acl.c 2007-10-09 15:31:38.000000000 -0500
> > > +++ b/fs/nfsd/nfs2acl.c 2007-10-24 14:03:13.000000000 -0500
> > > @@ -221,7 +221,7 @@ static int nfsaclsvc_encode_getaclres(st
> > > struct nfsd3_getaclres *resp)
> > > {
> > > struct dentry *dentry = resp->fh.fh_dentry;
> > > - struct inode *inode = dentry->d_inode;
> > > + struct inode *inode;
> > > struct kvec *head = rqstp->rq_res.head;
> > > unsigned int base;
> > > int n;
> > > --
> > > Thanks.
> > >
> > > Regards,
> > > Prasad Potluri
> > > email: pvp at-sign us dot ibm dot com
> > >
> -------------------------------------------------------------------------
> > > 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 - [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/nfs
> >
> >
> > -------------------------------------------------------------------------
> > 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 - [email protected]
> > https://lists.sourceforge.net/lists/listinfo/nfs
> Thanks.
>
> Regards,
> Prasad Potluri
> email: pvp at-sign us dot ibm dot com
> -------------------------------------------------------------------------
> 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 - [email protected]
> https://lists.sourceforge.net/lists/listinfo/nfs
-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> On Thu, Oct 25, 2007 at 10:26:14AM -0500, Prasad P wrote:
>
>> [email protected] 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.
-------------------------------------------------------------------------
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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:
> >
> >> [email protected] 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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs