fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

[Adrian Bunk]

The Coverity checker spotted the following:

<--  snip  -->

...
STATIC int
xfs_readdir(
        bhv_desc_t      *dir_bdp,
        uio_t           *uiop,
        cred_t          *credp,
        int             *eofp)
{
        xfs_inode_t     *dp;
        xfs_trans_t     *tp = NULL;
        int             error = 0;
        uint            lock_mode;

        vn_trace_entry(BHV_TO_VNODE(dir_bdp), __FUNCTION__,
                                               (inst_t *)__return_address);
        dp = XFS_BHVTOI(dir_bdp);

        if (XFS_FORCED_SHUTDOWN(dp->i_mount))
                return XFS_ERROR(EIO);

        lock_mode = xfs_ilock_map_shared(dp);
        error = xfs_dir_getdents(tp, dp, uiop, eofp);
        xfs_iunlock_map_shared(dp, lock_mode);
        return error;
}
...

<--  snip  -->

Note that tp is never assigned any value other than NULL (and the 
Coverity checker found a way how tp might be dereferenced four function 
calls later).

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

[David Chinner]

On Thu, Jul 06, 2006 at 11:13:20PM +0200, Adrian Bunk wrote:
> The Coverity checker spotted the following:
> 
> <--  snip  -->
> 
> ...
> STATIC int
> xfs_readdir(
>         bhv_desc_t      *dir_bdp,
>         uio_t           *uiop,
>         cred_t          *credp,
>         int             *eofp)
> {
>         xfs_inode_t     *dp;
>         xfs_trans_t     *tp = NULL;
>         int             error = 0;
>         uint            lock_mode;
> 
>         vn_trace_entry(BHV_TO_VNODE(dir_bdp), __FUNCTION__,
>                                                (inst_t *)__return_address);
>         dp = XFS_BHVTOI(dir_bdp);
> 
>         if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>                 return XFS_ERROR(EIO);
> 
>         lock_mode = xfs_ilock_map_shared(dp);
>         error = xfs_dir_getdents(tp, dp, uiop, eofp);
>         xfs_iunlock_map_shared(dp, lock_mode);
>         return error;
> }
> ...
> 
> <--  snip  -->
> 
> Note that tp is never assigned any value other than NULL (and the 
> Coverity checker found a way how tp might be dereferenced four function 
> calls later).

Then the bug is probably in the function call that uses tp without
first checking whether it's null. Can you tell us where that dereference
occurs?

Cheers,

Dave.

-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

RE: fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

[Hua Zhong]

> > <--  snip  -->
> > 
> > Note that tp is never assigned any value other than NULL (and the 
> > Coverity checker found a way how tp might be dereferenced four 
> > function calls later).
> 
> Then the bug is probably in the function call that uses tp 
> without first checking whether it's null. Can you tell us 
> where that dereference occurs?
> 
> Cheers,
> 
> Dave.

Maybe, but the above code is confusing too.

Why not get rid of tp and explicitly pass NULL as "xfs_dir_getdents(NULL, dp, uiop, eofp);"?

Hua

Re: fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

[Adrian Bunk]

On Fri, Jul 07, 2006 at 09:32:46AM +1000, David Chinner wrote:
> On Thu, Jul 06, 2006 at 11:13:20PM +0200, Adrian Bunk wrote:
> > The Coverity checker spotted the following:
> > 
> > <--  snip  -->
> > 
> > ...
> > STATIC int
> > xfs_readdir(
> >         bhv_desc_t      *dir_bdp,
> >         uio_t           *uiop,
> >         cred_t          *credp,
> >         int             *eofp)
> > {
> >         xfs_inode_t     *dp;
> >         xfs_trans_t     *tp = NULL;
> >         int             error = 0;
> >         uint            lock_mode;
> > 
> >         vn_trace_entry(BHV_TO_VNODE(dir_bdp), __FUNCTION__,
> >                                                (inst_t *)__return_address);
> >         dp = XFS_BHVTOI(dir_bdp);
> > 
> >         if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> >                 return XFS_ERROR(EIO);
> > 
> >         lock_mode = xfs_ilock_map_shared(dp);
> >         error = xfs_dir_getdents(tp, dp, uiop, eofp);
> >         xfs_iunlock_map_shared(dp, lock_mode);
> >         return error;
> > }
> > ...
> > 
> > <--  snip  -->
> > 
> > Note that tp is never assigned any value other than NULL (and the 
> > Coverity checker found a way how tp might be dereferenced four function 
> > calls later).
> 
> Then the bug is probably in the function call that uses tp without
> first checking whether it's null. Can you tell us where that dereference
> occurs?

xfs_readdir()
  xfs_dir_getdents()
    xfs_dir2_leaf_getdents()
      xfs_bmapi()
        xfs_trans_log_inode()
          tp->t_flags |= XFS_TRANS_DIRTY;

> Cheers,
> 
> Dave.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [xfs-masters] Re: fs/xfs/xfs_vnodeops.c:xfs_readdir(): NULL variable dereferenced

[Nathan Scott]

On Sat, Jul 08, 2006 at 09:46:09PM +0200, Adrian Bunk wrote:
> On Fri, Jul 07, 2006 at 09:32:46AM +1000, David Chinner wrote:
> > > Coverity checker found a way how tp might be dereferenced four function 
> > > calls later).

Keyword being "might". ;)

> > Then the bug is probably in the function call that uses tp without
> > first checking whether it's null. Can you tell us where that dereference
> > occurs?
> 
> xfs_readdir()
>   xfs_dir_getdents()
>     xfs_dir2_leaf_getdents()
>       xfs_bmapi()
>         xfs_trans_log_inode()
>           tp->t_flags |= XFS_TRANS_DIRTY;

This actually cant happen due to the flags passed into xfs_bmapi (ie.
request for a extent map _read_, which wont result in the inode being
logged, which means no transaction dirtying).

That suggestion to remove the local "tp" variable was valid though,
we may as well do that (will do).

cheers.

-- 
Nathan