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

[Emmanuel Dreyfus]

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

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