2006-03-27 18:53:56

by Frank Filz

[permalink] [raw]
Subject: [Patch 4/4] Coverity #324

Signed-off by: Frank Filz

diff --git a/include/linux/nfsd/nfsfh.h b/include/linux/nfsd/nfsfh.h
--- a/include/linux/nfsd/nfsfh.h
+++ b/include/linux/nfsd/nfsfh.h
@@ -328,7 +328,7 @@ fh_lock(struct svc_fh *fhp)
static inline void
fh_unlock(struct svc_fh *fhp)
{
- if (!fhp->fh_dentry)
+ if (!fhp || !fhp->fh_dentry)
printk(KERN_ERR "fh_unlock: fh not verified!\n");

if (fhp->fh_locked) {




-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2006-03-30 20:37:16

by Frank Filz

[permalink] [raw]
Subject: Resubmit: [Patch 4/4] Coverity #324 and Coverity #323

On Mon, 2006-03-27 at 14:08 -0500, Trond Myklebust wrote:
> Secondly, if fhp is NULL, then we really _ought_ to Oops in fh_unlock(),
> 'cos that would be a nasty evil coding ERROR that needs to be analysed
> and debugged.

Ok, I re-examined this. Looking at fh_lock, I see that the correct
behavior is to return without locking when fh_dentry is NULL. I've also
fixed Coverity #323, which is a similar issue in the same source file.

Frank

Signed-of by: Frank Filz

diff --git a/include/linux/nfsd/nfsfh.h b/include/linux/nfsd/nfsfh.h
--- a/include/linux/nfsd/nfsfh.h
+++ b/include/linux/nfsd/nfsfh.h
@@ -218,7 +218,7 @@ void fh_put(struct svc_fh *);
static __inline__ struct svc_fh *
fh_copy(struct svc_fh *dst, struct svc_fh *src)
{
- if (src->fh_dentry || src->fh_locked) {
+ if (src->fh_dentry && src->fh_locked) {
struct dentry *dentry = src->fh_dentry;
printk(KERN_ERR "fh_copy: copying %s/%s, already verified!\n",
dentry->d_parent->d_name.name, dentry->d_name.name);
@@ -328,8 +328,10 @@ fh_lock(struct svc_fh *fhp)
static inline void
fh_unlock(struct svc_fh *fhp)
{
- if (!fhp->fh_dentry)
+ if (!fhp->fh_dentry) {
printk(KERN_ERR "fh_unlock: fh not verified!\n");
+ return;
+ }

if (fhp->fh_locked) {
fill_post_wcc(fhp);




-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2006-03-27 19:09:12

by Trond Myklebust

[permalink] [raw]
Subject: Re: [Patch 4/4] Coverity #324

On Mon, 2006-03-27 at 10:57 -0800, Frank Filz wrote:
> Signed-off by: Frank Filz
>
> diff --git a/include/linux/nfsd/nfsfh.h b/include/linux/nfsd/nfsfh.h
> --- a/include/linux/nfsd/nfsfh.h
> +++ b/include/linux/nfsd/nfsfh.h
> @@ -328,7 +328,7 @@ fh_lock(struct svc_fh *fhp)
> static inline void
> fh_unlock(struct svc_fh *fhp)
> {
> - if (!fhp->fh_dentry)
> + if (!fhp || !fhp->fh_dentry)
> printk(KERN_ERR "fh_unlock: fh not verified!\n");
>
> if (fhp->fh_locked) {

Huh???

Firstly, if fhp is NULL, then fhp->fh_locked is going to Oops too.

Secondly, if fhp is NULL, then we really _ought_ to Oops in fh_unlock(),
'cos that would be a nasty evil coding ERROR that needs to be analysed
and debugged.

Cheers,
Trond



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2006-03-27 19:40:34

by Frank Filz

[permalink] [raw]
Subject: Re: [Patch 4/4] Coverity #324

On Mon, 2006-03-27 at 14:08 -0500, Trond Myklebust wrote:
> On Mon, 2006-03-27 at 10:57 -0800, Frank Filz wrote:
> > Signed-off by: Frank Filz
> >
> > diff --git a/include/linux/nfsd/nfsfh.h b/include/linux/nfsd/nfsfh.h
> > --- a/include/linux/nfsd/nfsfh.h
> > +++ b/include/linux/nfsd/nfsfh.h
> > @@ -328,7 +328,7 @@ fh_lock(struct svc_fh *fhp)
> > static inline void
> > fh_unlock(struct svc_fh *fhp)
> > {
> > - if (!fhp->fh_dentry)
> > + if (!fhp || !fhp->fh_dentry)
> > printk(KERN_ERR "fh_unlock: fh not verified!\n");
> >
> > if (fhp->fh_locked) {
>
> Huh???
>
> Firstly, if fhp is NULL, then fhp->fh_locked is going to Oops too.
>
> Secondly, if fhp is NULL, then we really _ought_ to Oops in fh_unlock(),
> 'cos that would be a nasty evil coding ERROR that needs to be analysed
> and debugged.

Oops, I must have been half asleep there.

I misread the Coverity error. What it was actually complaining about was
that the code tested for NULL fh_dentry and then passed it to
fill_post_wcc, not to mention that right after the call to
fill_post_wcc, it dereferences fh_dentry.

Back to the drawing board on this one.

Frank




-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs