[Gluster-devel] Invalid DIR * usage in quota xlator

[J. Bruce Fields]

On Mon, Oct 13, 2014 at 08:57:11AM +0000, Emmanuel Dreyfus wrote:
> On Mon, Oct 13, 2014 at 01:42:38PM +0530, Pranith Kumar Karampuri wrote:
> > >No bug here, just suboptimal behavior, both in glusterfs and NetBSD FUSE.
> > oh!, but shouldn't it get op_ret = 0 instead of op_ret -1, op_errno EINVAL?
> 
> It happens because of thei change I introduced:
>                 seekdir (dir, (long)off);
> #ifndef GF_LINUX_HOST_OS
>                 if (telldir(dir) != off) {
>                         gf_log (THIS->name, GF_LOG_ERROR,
>                                 "seekdir(%ld) failed on dir=%p: "
>                                 "Invalid argument (offset reused from "
>                                 "another DIR * structure?)", (long)off, dir);
>                         errno = EINVAL;
>                         count = -1;
>                         goto out;
>                 }   
> #endif /* GF_LINUX_HOST_OS */
> 
> The idea is to catch wrong opendir/closedir usage, and the errno = EINVAL
> is critical in such a situation to avoid an infinite loop. On the other
> hand it also fires for last entry. I did not notice it before because
> this EINVAL will not reach calling process, but it may cause trouble
> for internal glusterfs usage.
> 
> I see an easy way yo fix it (patch below)
> - when posix xlator detects EOF, return an offset of -1 in last entry.
> - When called with an offset of -1, immediatly return op_ret = 0 with no entry

You're assuming no filesystem uses -1 as a cookie?

Might be true, but it's taking a small chance.

--b.

> 
> What do you think?
> diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix
> index 6b12d39..3e539bb 100644
> --- a/xlators/storage/posix/src/posix.c
> +++ b/xlators/storage/posix/src/posix.c
> @@ -79,6 +79,10 @@ extern char *marker_xattrs[];
>  #define SET_TO_OLD_FS_ID()
>  
>  #endif
> +
> +/* offset < sizeof(struct dirent) cannot be valid */
> +#define INVALID_DIRENT_OFFSET sizeof (void *)
> +
>  int
>  posix_forget (xlator_t *this, inode_t *inode)
>  {
> @@ -4851,6 +4855,10 @@ posix_fill_readdir (fd_t *fd, DIR *dir, off_t off, size_t
>          int                   len = 0;
>          int                   ret = 0;
>  
> +        /* EOF was previously detected */
> +        if (off == INVALID_DIRENT_OFFSET) 
> +               goto out;
> +
>          if (skip_dirs) {
>                  len = posix_handle_path (this, fd->inode->gfid, NULL, NULL, 0);
>                  hpath = alloca (len + 256); /* NAME_MAX */
> @@ -4969,9 +4977,12 @@ posix_fill_readdir (fd_t *fd, DIR *dir, off_t off, size_t
>                  count ++;
>          }
>  
> -        if ((!readdir (dir) && (errno == 0)))
> +        if ((!readdir (dir) && (errno == 0))) {
>                  /* Indicate EOF */
>                  errno = ENOENT;
> +                /* Set invalid offset for later detection */
> +                this_entry->d_off = INVALID_DIRENT_OFFSET;
> +        }
>  out:
>          return count;
>  }
> 
> 
> -- 
> Emmanuel Dreyfus
> manu at netbsd.org
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel