* [PATCH] Fix incorrect assignment
@ 2007-10-24 20:14 Prasad P
2007-10-24 20:57 ` J. Bruce Fields
0 siblings, 1 reply; 6+ messages in thread
From: Prasad P @ 2007-10-24 20:14 UTC (permalink / raw)
To: nfs
[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]
Dereferenced pointer "dentry" without checking and assigned to inode
in the declaration.
Cc: Steve French <smfltc@us.ibm.com>
Signed-off-by: Prasad V Potluri <pvp@us.ibm.com>
---
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
[-- Attachment #1.2: Type: text/html, Size: 861 bytes --]
[-- Attachment #2: Type: text/plain, Size: 314 bytes --]
-------------------------------------------------------------------------
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/
[-- Attachment #3: Type: text/plain, Size: 140 bytes --]
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix incorrect assignment
2007-10-24 20:14 [PATCH] Fix incorrect assignment Prasad P
@ 2007-10-24 20:57 ` J. Bruce Fields
2007-10-25 15:26 ` Prasad P
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2007-10-24 20:57 UTC (permalink / raw)
To: Prasad P; +Cc: nfs
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 <smfltc@us.ibm.com>
> Signed-off-by: Prasad V Potluri <pvp@us.ibm.com>
> ---
>
> 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 - NFS@lists.sourceforge.net
> 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 - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix incorrect assignment
2007-10-24 20:57 ` J. Bruce Fields
@ 2007-10-25 15:26 ` Prasad P
2007-10-25 16:03 ` J. Bruce Fields
0 siblings, 1 reply; 6+ messages in thread
From: Prasad P @ 2007-10-25 15:26 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: nfs-bounces, nfs, smfltc
[-- Attachment #1.1: Type: text/plain, Size: 2615 bytes --]
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;
>
>
> >
> > Cc: Steve French <smfltc@us.ibm.com>
> > Signed-off-by: Prasad V Potluri <pvp@us.ibm.com>
> > ---
> >
> > 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 - NFS@lists.sourceforge.net
> > 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 - NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs
Thanks.
Regards,
Prasad Potluri
email: pvp at-sign us dot ibm dot com
[-- Attachment #1.2: Type: text/html, Size: 3737 bytes --]
[-- Attachment #2: Type: text/plain, Size: 314 bytes --]
-------------------------------------------------------------------------
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/
[-- Attachment #3: Type: text/plain, Size: 140 bytes --]
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix incorrect assignment
2007-10-25 15:26 ` Prasad P
@ 2007-10-25 16:03 ` J. Bruce Fields
2007-10-25 17:58 ` Steve French
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2007-10-25 16:03 UTC (permalink / raw)
To: Prasad P; +Cc: nfs-bounces, nfs, smfltc
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.
--b.
>
> >
> >
> > >
> > > Cc: Steve French <smfltc@us.ibm.com>
> > > Signed-off-by: Prasad V Potluri <pvp@us.ibm.com>
> > > ---
> > >
> > > 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 - NFS@lists.sourceforge.net
> > > 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 - NFS@lists.sourceforge.net
> > 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 - NFS@lists.sourceforge.net
> 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 - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix incorrect assignment
2007-10-25 16:03 ` J. Bruce Fields
@ 2007-10-25 17:58 ` Steve French
2007-10-25 23:12 ` J. Bruce Fields
0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2007-10-25 17:58 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Prasad P, nfs
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.
-------------------------------------------------------------------------
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix incorrect assignment
2007-10-25 17:58 ` Steve French
@ 2007-10-25 23:12 ` J. Bruce Fields
0 siblings, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2007-10-25 23:12 UTC (permalink / raw)
To: Steve French; +Cc: Prasad P, 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:
> >
> >> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-10-25 23:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-24 20:14 [PATCH] Fix incorrect assignment Prasad P
2007-10-24 20:57 ` J. Bruce Fields
2007-10-25 15:26 ` Prasad P
2007-10-25 16:03 ` J. Bruce Fields
2007-10-25 17:58 ` Steve French
2007-10-25 23:12 ` J. Bruce Fields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.