storage/posix: Don't allow gfid/volume-id xattr to be removed

Problem:
Bulk xattr removal doesn't check if the xattrs that are coming in xdata
have gfid/volume-id xattrs, so there is potential for bulkremovexattr
removing gfid/volume-id.
I also observed that bulkremovexattr is not available for fremovexattr.

Fix:
Do proper checks in bulk removexattr to remove gfid/volume-id.
Refactor [f]removexattr to reduce the differences.

BUG: 1470489
Change-Id: Ia845b31846a149500111c0996646e648f72cdce6
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
Reviewed-on: https://review.gluster.org/17765
Smoke: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Jeff Darcy <jeff@pl.atyp.us>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Anuradha Talur <atalur@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Reviewed-by: Krutika Dhananjay <kdhananj@redhat.com>

3 files changed, 134 insertions, 140 deletions

diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c
index 8c1a789f70b..53c8a86101c 100644
--- a/xlators/storage/posix/src/posix-helpers.c
+++ b/xlators/storage/posix/src/posix-helpers.c
@@ -148,26 +148,10 @@ out:
         return ret;
 }
 
-
-static gf_boolean_t
-_is_in_array (char **str_array, char *str)
-{
-        int i = 0;
-
-        if (!str)
-                return _gf_false;
-
-        for (i = 0; str_array[i]; i++) {
-                if (strcmp (str, str_array[i]) == 0)
-                        return _gf_true;
-        }
-        return _gf_false;
-}
-
 static gf_boolean_t
 posix_xattr_ignorable (char *key)
 {
-        return _is_in_array (posix_ignore_xattrs, key);
+        return gf_get_index_by_elem (posix_ignore_xattrs, key) >= 0;
 }
 
 static int
@@ -763,7 +747,7 @@ _handle_list_xattr (dict_t *xattr_req, const char *real_path, int fdnum,
         while (remaining_size > 0) {
                 key = list + list_offset;
 
-                if (_is_in_array (list_xattr_ignore_xattrs, key))
+                if (gf_get_index_by_elem (list_xattr_ignore_xattrs, key) >= 0)
                         goto next;
 
                 if (posix_special_xattr (marker_xattrs, key))
@@ -2359,3 +2343,11 @@ posix_inode_ctx_get_all (inode_t *inode, xlator_t *this,
 
         return ret;
 }
+
+gf_boolean_t
+posix_is_bulk_removexattr (char *name, dict_t *xdata)
+{
+        if (name && (strlen (name) == 0) && xdata)
+                return _gf_true;
+        return _gf_false;
+}
diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c
index 5f65172cdb8..e3fa184c1f2 100644
--- a/xlators/storage/posix/src/posix.c
+++ b/xlators/storage/posix/src/posix.c
@@ -103,6 +103,12 @@ extern char *marker_xattrs[];
         (lutimes (path, tv))
 #endif
 
+static char *disallow_removexattrs[] = {
+        GF_XATTR_VOL_ID_KEY,
+        GFID_XATTR_KEY,
+        NULL
+};
+
 
 dict_t*
 posix_dict_set_nlink (dict_t *req, dict_t *res, int32_t nlink)
@@ -5350,7 +5356,11 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data)
      * treated as success.
      */
 
-        op_ret = sys_lremovexattr (filler->real_path, key);
+        if (filler->real_path)
+                op_ret = sys_lremovexattr (filler->real_path, key);
+        else
+                op_ret = sys_fremovexattr (filler->fdnum, key);
+
         if (op_ret == -1) {
                 if (errno == ENODATA || errno == ENOATTR)
                         op_ret = 0;
@@ -5358,10 +5368,13 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data)
 
         if (op_ret == -1) {
                 filler->op_errno = errno;
-                if (errno != ENOATTR && errno != ENODATA && errno != EPERM)
+                if (errno != ENOATTR && errno != ENODATA && errno != EPERM) {
                         gf_msg (this->name, GF_LOG_ERROR, errno,
-                                P_MSG_XATTR_FAILED, "removexattr failed on %s"
-                                " (for %s)", filler->real_path, key);
+                                P_MSG_XATTR_FAILED, "removexattr failed on "
+                                "file/dir %s with gfid: %s (for %s)",
+                                filler->real_path?filler->real_path:"",
+                                uuid_utoa (filler->inode->gfid), key);
+                }
         }
 #ifdef GF_DARWIN_HOST_OS
         GF_FREE(newkey);
@@ -5369,92 +5382,132 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data)
         return op_ret;
 }
 
-
-int32_t
-posix_removexattr (call_frame_t *frame, xlator_t *this,
-                   loc_t *loc, const char *name, dict_t *xdata)
+int
+posix_common_removexattr (call_frame_t *frame, loc_t *loc, fd_t *fd,
+                          const char *name, dict_t *xdata, int *op_errno,
+                          dict_t **xdata_rsp)
 {
-        int32_t op_ret    = -1;
-        int32_t op_errno  = 0;
-        int32_t ret    = -1;
-        char *  real_path = NULL;
-        struct iatt   stbuf         = {0};
-        dict_t  *xattr    = NULL;
-        posix_xattr_filler_t filler = {0,};
+        gf_boolean_t         bulk_removexattr = _gf_false;
+        gf_boolean_t         disallow         = _gf_false;
+        char                 *real_path       = NULL;
+        struct posix_fd      *pfd             = NULL;
+        int                  op_ret           = 0;
+        struct iatt          stbuf            = {0};
+        int                  ret              = 0;
+        int                  _fd              = -1;
+        xlator_t             *this            = frame->this;
+        inode_t              *inode           = NULL;
+        posix_xattr_filler_t filler           = {0};
 
         DECLARE_OLD_FS_ID_VAR;
 
-        MAKE_INODE_HANDLE (real_path, this, loc, NULL);
-        if (!real_path) {
-                op_ret = -1;
-                op_errno = ESTALE;
-                goto out;
-        }
-
+        SET_FS_ID (frame->root->uid, frame->root->gid);
 
-        if (!strcmp (GFID_XATTR_KEY, name)) {
-                gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED,
-                        "Remove xattr called on gfid for file %s", real_path);
-                op_ret = -1;
-                goto out;
+        if (loc) {
+                MAKE_INODE_HANDLE (real_path, this, loc, NULL);
+                if (!real_path) {
+                        op_ret = -1;
+                        *op_errno = ESTALE;
+                        goto out;
+                }
+                inode = loc->inode;
+        } else {
+                op_ret = posix_fd_ctx_get (fd, this, &pfd, op_errno);
+                if (op_ret < 0) {
+                        gf_msg (this->name, GF_LOG_WARNING, *op_errno,
+                                P_MSG_PFD_NULL, "pfd is NULL from fd=%p", fd);
+                        goto out;
+                }
+                _fd = pfd->fd;
+                inode = fd->inode;
         }
-        if (!strcmp (GF_XATTR_VOL_ID_KEY, name)) {
+
+        if (gf_get_index_by_elem (disallow_removexattrs, (char *)name) >= 0) {
                 gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED,
-                        "Remove xattr called on volume-id for file %s",
-                        real_path);
+                        "Remove xattr called on %s for file/dir %s with gfid: "
+                        "%s", name, real_path?real_path:"",
+                        uuid_utoa(inode->gfid));
                 op_ret = -1;
+                *op_errno = EPERM;
                 goto out;
+        } else if (posix_is_bulk_removexattr ((char *)name, xdata)) {
+                bulk_removexattr = _gf_true;
+                (void) dict_has_key_from_array (xdata, disallow_removexattrs,
+                                                &disallow);
+                if (disallow) {
+                        gf_msg (this->name, GF_LOG_WARNING, 0,
+                                P_MSG_XATTR_NOT_REMOVED,
+                                "Bulk removexattr has keys that shouldn't be "
+                                "removed for file/dir %s with gfid: %s",
+                                real_path?real_path:"", uuid_utoa(inode->gfid));
+                        op_ret = -1;
+                        *op_errno = EPERM;
+                        goto out;
+                }
         }
 
-
-        SET_FS_ID (frame->root->uid, frame->root->gid);
-
-        /**
-         * sending an empty key name with xdata containing the
-         * list of key(s) to be removed implies "bulk remove request"
-         * for removexattr.
-         */
-        if (name && (strcmp (name, "") == 0) && xdata) {
+        if (bulk_removexattr) {
                 filler.real_path = real_path;
                 filler.this = this;
+                filler.fdnum = _fd;
+                filler.inode = inode;
                 op_ret = dict_foreach (xdata, _posix_remove_xattr, &filler);
                 if (op_ret) {
-                        op_errno = filler.op_errno;
+                        *op_errno = filler.op_errno;
+                        goto out;
+                }
+        } else {
+                if (loc)
+                        op_ret = sys_lremovexattr (real_path, name);
+                else
+                        op_ret = sys_fremovexattr (_fd, name);
+                if (op_ret == -1) {
+                        *op_errno = errno;
+                        if (*op_errno != ENOATTR && *op_errno != ENODATA &&
+                            *op_errno != EPERM) {
+                                gf_msg (this->name, GF_LOG_ERROR, *op_errno,
+                                        P_MSG_XATTR_FAILED,
+                                        "removexattr on %s with gfid %s "
+                                        "(for %s)", real_path,
+                                        uuid_utoa (inode->gfid), name);
+                        }
+                        goto out;
                 }
-
-                goto out;
-        }
-
-        op_ret = sys_lremovexattr (real_path, name);
-        if (op_ret == -1) {
-                op_errno = errno;
-                if (op_errno != ENOATTR && op_errno != ENODATA &&
-                    op_errno != EPERM)
-                        gf_msg (this->name, GF_LOG_ERROR, errno,
-                                P_MSG_XATTR_FAILED, "removexattr on %s "
-                                "(for %s)", real_path, name);
-                goto out;
         }
 
         if (xdata && dict_get (xdata, DHT_IATT_IN_XDATA_KEY)) {
-                ret = posix_pstat(this, loc->gfid, real_path, &stbuf);
+                if (loc)
+                        ret = posix_pstat(this, loc->gfid, real_path, &stbuf);
+                else
+                        ret = posix_fdstat (this, _fd, &stbuf);
                 if (ret)
                         goto out;
-                xattr = dict_new();
-                if (!xattr)
+                *xdata_rsp = dict_new();
+                if (!*xdata_rsp)
                         goto out;
 
-                ret = posix_set_iatt_in_dict (xattr, &stbuf);
+                ret = posix_set_iatt_in_dict (*xdata_rsp, &stbuf);
         }
         op_ret = 0;
-
 out:
         SET_TO_OLD_FS_ID ();
+        return op_ret;
+}
 
-        STACK_UNWIND_STRICT (removexattr, frame, op_ret, op_errno, xattr);
+int32_t
+posix_removexattr (call_frame_t *frame, xlator_t *this,
+                   loc_t *loc, const char *name, dict_t *xdata)
+{
+        int    op_ret     = 0;
+        int    op_errno   = 0;
+        dict_t *xdata_rsp = NULL;
 
-        if (xattr)
-                dict_unref (xattr);
+        op_ret = posix_common_removexattr (frame, loc, NULL, name, xdata,
+                                           &op_errno, &xdata_rsp);
+        STACK_UNWIND_STRICT (removexattr, frame, op_ret, op_errno, xdata_rsp);
+
+        if (xdata_rsp)
+                dict_unref (xdata_rsp);
 
         return 0;
 }
@@ -5463,69 +5516,16 @@ int32_t
 posix_fremovexattr (call_frame_t *frame, xlator_t *this,
                     fd_t *fd, const char *name, dict_t *xdata)
 {
-        int32_t           op_ret   = -1;
-        int32_t           op_errno = 0;
-        struct posix_fd * pfd      = NULL;
-        struct iatt       stbuf    = {0,};
-        dict_t           *xattr    = NULL;
-        int               _fd      = -1;
-        int               ret      = -1;
-
-        DECLARE_OLD_FS_ID_VAR;
+        int32_t op_ret     = 0;
+        int32_t op_errno   = 0;
+        dict_t  *xdata_rsp = NULL;
 
-        if (!strcmp (GFID_XATTR_KEY, name)) {
-                gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED,
-                        "Remove xattr called on gfid for file");
-                goto out;
-        }
-        if (!strcmp (GF_XATTR_VOL_ID_KEY, name)) {
-                gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED,
-                        "Remove xattr called on volume-id for file");
-                goto out;
-        }
+        op_ret = posix_common_removexattr (frame, NULL, fd, name, xdata,
+                                           &op_errno, &xdata_rsp);
+        STACK_UNWIND_STRICT (fremovexattr, frame, op_ret, op_errno, xdata_rsp);
 
-        ret = posix_fd_ctx_get (fd, this, &pfd, &op_errno);
-        if (ret < 0) {
-                gf_msg (this->name, GF_LOG_WARNING, op_errno, P_MSG_PFD_NULL,
-                        "pfd is NULL from fd=%p", fd);
-                goto out;
-        }
-        _fd = pfd->fd;
-
-
-
-        SET_FS_ID (frame->root->uid, frame->root->gid);
-
-        op_ret = sys_fremovexattr (_fd, name);
-        if (op_ret == -1) {
-                op_errno = errno;
-                if (op_errno != ENOATTR && op_errno != ENODATA &&
-                    op_errno != EPERM)
-                        gf_msg (this->name, GF_LOG_ERROR, errno,
-                                P_MSG_XATTR_FAILED, "fremovexattr (for %s)",
-                                name);
-                goto out;
-        }
-
-        if (xdata && dict_get (xdata, DHT_IATT_IN_XDATA_KEY)) {
-                ret = posix_fdstat (this, pfd->fd, &stbuf);
-                if (ret)
-                        goto out;
-                xattr = dict_new();
-                if (!xattr)
-                        goto out;
-
-                ret = posix_set_iatt_in_dict (xattr, &stbuf);
-        }
-        op_ret = 0;
-
-out:
-        SET_TO_OLD_FS_ID ();
-
-        STACK_UNWIND_STRICT (fremovexattr, frame, op_ret, op_errno, xattr);
-
-        if (xattr)
-                dict_unref (xattr);
+        if (xdata_rsp)
+                dict_unref (xdata_rsp);
 
         return 0;
 }
diff --git a/xlators/storage/posix/src/posix.h b/xlators/storage/posix/src/posix.h
index 332c0ab4f49..480566a5340 100644
--- a/xlators/storage/posix/src/posix.h
+++ b/xlators/storage/posix/src/posix.h
@@ -293,4 +293,6 @@ posix_get_objectsignature (char *, dict_t *);
 int32_t
 posix_fdget_objectsignature (int, dict_t *);
 
+gf_boolean_t
+posix_is_bulk_removexattr (char *name, dict_t *dict);
 #endif /* _POSIX_H */