storage/posix: handle getxattr failures gracefully

Use proper variable types for getting return value of getxattr calls,
which otherwise can lead to segfaulting of processes or page allocation
failures in the kernel.

Change-Id: Idc41b4022401c238d17ba357648234f7c2d56c87
BUG: 838195
Signed-off-by: Raghavendra Bhat <raghavendra@redhat.com>
Reviewed-on: http://review.gluster.com/3658
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Vijay Bellur <vbellur@redhat.com>

2 files changed, 124 insertions, 52 deletions

diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c
index 06b5cedcb12..f4dcc70a2ab 100644
--- a/xlators/storage/posix/src/posix-helpers.c
+++ b/xlators/storage/posix/src/posix-helpers.c
@@ -214,16 +214,25 @@ _posix_xattr_get_set (dict_t *xattr_req,
                         if (!value)
                                 return;
 
-                        sys_lgetxattr (filler->real_path, key, value,
-                                       xattr_size);
+                        xattr_size = sys_lgetxattr (filler->real_path, key,
+                                                    value, xattr_size);
+                        if (xattr_size <= 0) {
+                                gf_log (filler->this->name, GF_LOG_WARNING,
+                                        "getxattr failed. path: %s, key: %s",
+                                        filler->real_path, key);
+                                GF_FREE (value);
+                                return;
+                        }
 
                         value[xattr_size] = '\0';
                         ret = dict_set_bin (filler->xattr, key,
                                             value, xattr_size);
-                        if (ret < 0)
+                        if (ret < 0) {
                                 gf_log (filler->this->name, GF_LOG_DEBUG,
                                         "dict set failed. path: %s, key: %s",
                                         filler->real_path, key);
+                                GF_FREE (value);
+                        }
                 }
         }
 out:
@@ -235,14 +244,17 @@ int
 posix_fill_gfid_path (xlator_t *this, const char *path, struct iatt *iatt)
 {
         int ret = 0;
+        ssize_t  size = 0;
 
         if (!iatt)
                 return 0;
 
-        ret = sys_lgetxattr (path, GFID_XATTR_KEY, iatt->ia_gfid, 16);
+        size = sys_lgetxattr (path, GFID_XATTR_KEY, iatt->ia_gfid, 16);
         /* Return value of getxattr */
-        if ((ret == 16) || (ret == -1))
+        if ((size == 16) || (size == -1))
                 ret = 0;
+        else
+                ret = size;
 
         return ret;
 }
@@ -252,14 +264,17 @@ int
 posix_fill_gfid_fd (xlator_t *this, int fd, struct iatt *iatt)
 {
         int ret = 0;
+        ssize_t  size = 0;
 
         if (!iatt)
                 return 0;
 
-        ret = sys_fgetxattr (fd, GFID_XATTR_KEY, iatt->ia_gfid, 16);
+        size = sys_fgetxattr (fd, GFID_XATTR_KEY, iatt->ia_gfid, 16);
         /* Return value of getxattr */
-        if ((ret == 16) || (ret == -1))
+        if ((size == 16) || (size == -1))
                 ret = 0;
+        else
+                ret = size;
 
         return ret;
 }
@@ -443,6 +458,7 @@ posix_gfid_set (xlator_t *this, const char *path, loc_t *loc, dict_t *xattr_req)
         void        *uuid_req = NULL;
         uuid_t       uuid_curr;
         int          ret = 0;
+        ssize_t      size = 0;
         struct stat  stat = {0, };
 
 
@@ -452,8 +468,8 @@ posix_gfid_set (xlator_t *this, const char *path, loc_t *loc, dict_t *xattr_req)
         if (sys_lstat (path, &stat) != 0)
                 goto out;
 
-        ret = sys_lgetxattr (path, GFID_XATTR_KEY, uuid_curr, 16);
-        if (ret == 16) {
+        size = sys_lgetxattr (path, GFID_XATTR_KEY, uuid_curr, 16);
+        if (size == 16) {
                 ret = 0;
                 goto verify_handle;
         }
diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c
index 24fd6a253a1..63000a35120 100644
--- a/xlators/storage/posix/src/posix.c
+++ b/xlators/storage/posix/src/posix.c
@@ -2432,7 +2432,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
         int32_t               op_ret         = -1;
         int32_t               op_errno       = 0;
         int32_t               list_offset    = 0;
-        size_t                size           = 0;
+        ssize_t               size           = 0;
         size_t                remaining_size = 0;
         char     key[4096]                   = {0,};
         char     host_buf[1024]              = {0,};
@@ -2514,6 +2514,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
                         gf_log (this->name, GF_LOG_WARNING,
                                 "could not set value (%s) in dictionary",
                                 dyn_rpath);
+                        GF_FREE (dyn_rpath);
                 }
 
                 goto done;
@@ -2538,6 +2539,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
                         gf_log (this->name, GF_LOG_WARNING,
                                 "could not set value (%s) in dictionary",
                                 dyn_rpath);
+                        GF_FREE (dyn_rpath);
                 }
                 goto done;
         }
@@ -2556,6 +2558,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
                         gf_log (this->name, GF_LOG_WARNING,
                                 "could not set value (%s) in dictionary",
                                 host_buf);
+                        GF_FREE (path);
                 }
                 goto done;
         }
@@ -2586,14 +2589,22 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
                         op_ret = -1;
                         goto out;
                 }
-                op_ret = sys_lgetxattr (real_path, key, value, size);
-                if (op_ret == -1) {
+                size = sys_lgetxattr (real_path, key, value, size);
+                if (size == -1) {
+                        op_ret = -1;
                         op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "getxattr failed on "
+                                "%s: key = %s (%s)", real_path, key,
+                                strerror (op_errno));
+                        GF_FREE (value);
                         goto out;
                 }
-                value [op_ret] = '\0';
-                op_ret = dict_set_dynptr (dict, key, value, op_ret);
+                value [size] = '\0';
+                op_ret = dict_set_dynptr (dict, key, value, size);
                 if (op_ret < 0) {
+                        gf_log (this->name, GF_LOG_ERROR, "dict set operation "
+                                "on %s for the key %s failed.", real_path, key);
+                        GF_FREE (value);
                         goto out;
                 }
 
@@ -2637,26 +2648,40 @@ posix_getxattr (call_frame_t *frame, xlator_t *this,
                         break;
 
                 strcpy (key, list + list_offset);
-                op_ret = sys_lgetxattr (real_path, key, NULL, 0);
-                if (op_ret == -1)
+                size = sys_lgetxattr (real_path, key, NULL, 0);
+                if (size == -1) {
+                        op_ret = -1;
+                        op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "getxattr failed on "
+                                "%s: key = %s (%s)", real_path, key,
+                                strerror (op_errno));
                         break;
+                }
 
-                value = GF_CALLOC (op_ret + 1, sizeof(char),
+                value = GF_CALLOC (size + 1, sizeof(char),
                                    gf_posix_mt_char);
                 if (!value) {
                         op_errno = errno;
                         goto out;
                 }
 
-                op_ret = sys_lgetxattr (real_path, key, value, op_ret);
-                if (op_ret == -1) {
+                size = sys_lgetxattr (real_path, key, value, size);
+                if (size == -1) {
+                        op_ret = -1;
                         op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "getxattr failed on "
+                                "%s: key = %s (%s)", real_path, key,
+                                strerror (op_errno));
+                        GF_FREE (value);
                         break;
                 }
 
-                value [op_ret] = '\0';
-                op_ret = dict_set_dynptr (dict, key, value, op_ret);
+                value [size] = '\0';
+                op_ret = dict_set_dynptr (dict, key, value, size);
                 if (op_ret < 0) {
+                        gf_log (this->name, GF_LOG_ERROR, "dict set operation "
+                                "on %s for the key %s failed.", real_path, key);
+                        GF_FREE (value);
                         goto out;
                 }
 
@@ -2693,7 +2718,7 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this,
         struct posix_fd * pfd            = NULL;
         int               _fd            = -1;
         int32_t           list_offset    = 0;
-        size_t            size           = 0;
+        ssize_t           size           = 0;
         size_t            remaining_size = 0;
         char              key[4096]      = {0,};
         char *            value          = NULL;
@@ -2740,6 +2765,8 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this,
                 size = sys_fgetxattr (_fd, key, NULL, 0);
                 if (size <= 0) {
                         op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "fgetxattr failed on "
+                                "key %s (%s)", key, strerror (op_errno));
                         goto done;
                 }
 
@@ -2748,14 +2775,22 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this,
                         op_ret = -1;
                         goto out;
                 }
-                op_ret = sys_fgetxattr (_fd, key, value, op_ret);
-                if (op_ret == -1) {
+                size = sys_fgetxattr (_fd, key, value, size);
+                if (size == -1) {
+                        op_ret = -1;
                         op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "fgetxattr failed on "
+                                "fd %p for the key %s (%s)", fd, key,
+                                strerror (op_errno));
+                        GF_FREE (value);
                         goto out;
                 }
-                value [op_ret] = '\0';
-                op_ret = dict_set_dynptr (dict, key, value, op_ret);
+                value [size] = '\0';
+                op_ret = dict_set_dynptr (dict, key, value, size);
                 if (op_ret < 0) {
+                        gf_log (this->name, GF_LOG_ERROR, "dict set operation "
+                                "on key %s failed", key);
+                        GF_FREE (value);
                         goto out;
                 }
                 goto done;
@@ -2797,24 +2832,41 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this,
                         break;
 
                 strcpy (key, list + list_offset);
-                op_ret = sys_fgetxattr (_fd, key, NULL, 0);
-                if (op_ret == -1)
+                size = sys_fgetxattr (_fd, key, NULL, 0);
+                if (size == -1) {
+                        op_ret = -1;
+                        op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "fgetxattr failed on "
+                                "fd %p for the key %s (%s)", fd, key,
+                                strerror (op_errno));
                         break;
+                }
 
-                value = GF_CALLOC (op_ret + 1, sizeof(char),
+                value = GF_CALLOC (size + 1, sizeof(char),
                                    gf_posix_mt_char);
                 if (!value) {
+                        op_ret = -1;
                         op_errno = errno;
                         goto out;
                 }
 
-                op_ret = sys_fgetxattr (_fd, key, value, op_ret);
-                if (op_ret == -1)
+                size = sys_fgetxattr (_fd, key, value, size);
+                if (size == -1) {
+                        op_ret = -1;
+                        op_errno = errno;
+                        gf_log (this->name, GF_LOG_ERROR, "fgetxattr failed on "
+                                "the fd %p for the key %s (%s)", fd, key,
+                                strerror (op_errno));
+                        GF_FREE (value);
                         break;
+                }
 
-                value [op_ret] = '\0';
-                op_ret = dict_set_dynptr (dict, key, value, op_ret);
+                value [size] = '\0';
+                op_ret = dict_set_dynptr (dict, key, value, size);
                 if (op_ret) {
+                        gf_log (this->name, GF_LOG_ERROR, "dict set operation "
+                                "failed on key %s", key);
+                        GF_FREE (value);
                         goto out;
                 }
                 remaining_size -= strlen (key) + 1;
@@ -3892,6 +3944,7 @@ init (xlator_t *this)
         int                   dict_ret      = 0;
         int                   ret           = 0;
         int                   op_ret        = -1;
+        ssize_t               size          = -1;
         int32_t               janitor_sleep = 0;
         uuid_t                old_uuid      = {0,};
         uuid_t                dict_uuid     = {0,};
@@ -3978,9 +4031,9 @@ init (xlator_t *this)
                         ret = -1;
                         goto out;
                 }
-                op_ret = sys_lgetxattr (dir_data->data,
-                                        "trusted.glusterfs.volume-id", old_uuid, 16);
-                if (op_ret == 16) {
+                size = sys_lgetxattr (dir_data->data,
+                                      "trusted.glusterfs.volume-id", old_uuid, 16);
+                if (size == 16) {
                         if (uuid_compare (old_uuid, dict_uuid)) {
                                 gf_log (this->name, GF_LOG_ERROR,
                                         "mismatching volume-id (%s) received. "
@@ -3989,22 +4042,23 @@ init (xlator_t *this)
                                 ret = -1;
                                 goto out;
                         }
-                } else if ((op_ret == -1) && (errno == ENODATA)) {
+                } else if ((size == -1) && (errno == ENODATA)) {
                         /* Using the export for first time */
-                        op_ret = sys_lsetxattr (dir_data->data,
-                                                "trusted.glusterfs.volume-id",
-                                                dict_uuid, 16, 0);
-                        if (op_ret == -1) {
+                        size = sys_lsetxattr (dir_data->data,
+                                              "trusted.glusterfs.volume-id",
+                                              dict_uuid, 16, 0);
+                        if (size == -1) {
                                 gf_log (this->name, GF_LOG_ERROR,
                                         "failed to set volume id on export");
                                 ret = -1;
                                 goto out;
                         }
-                }  else if ((op_ret == -1) && (errno != ENODATA)) {
+                }  else if ((size == -1) && (errno != ENODATA)) {
                         /* Wrong 'volume-id' is set, it should be error */
                         gf_log (this->name, GF_LOG_WARNING,
                                 "%s: failed to fetch volume-id (%s)",
                                 dir_data->data, strerror (errno));
+                        ret = -1;
                         goto out;
                 } else {
                         ret = -1;
@@ -4016,8 +4070,8 @@ init (xlator_t *this)
 
         /* Now check if the export directory has some other 'gfid',
            other than that of root '/' */
-        ret = sys_lgetxattr (dir_data->data, "trusted.gfid", gfid, 16);
-        if (ret == 16) {
+        size = sys_lgetxattr (dir_data->data, "trusted.gfid", gfid, 16);
+        if (size == 16) {
                 if (!__is_root_gfid (gfid)) {
                         gf_log (this->name, GF_LOG_WARNING,
                                 "%s: gfid (%s) is not that of glusterfs '/' ",
@@ -4025,34 +4079,36 @@ init (xlator_t *this)
                         ret = -1;
                         goto out;
                 }
-        } else if (ret != -1) {
+        } else if (size != -1) {
                 /* Wrong 'gfid' is set, it should be error */
                 gf_log (this->name, GF_LOG_WARNING,
                         "%s: wrong value set as gfid",
                         dir_data->data);
                 ret = -1;
                 goto out;
-        } else if ((ret == -1) && (errno != ENODATA)) {
+        } else if ((size == -1) && (errno != ENODATA)) {
                 /* Wrong 'gfid' is set, it should be error */
                 gf_log (this->name, GF_LOG_WARNING,
                         "%s: failed to fetch gfid (%s)",
                         dir_data->data, strerror (errno));
+                ret = -1;
                 goto out;
         } else {
                 /* First time volume, set the GFID */
-                ret = sys_lsetxattr (dir_data->data, "trusted.gfid", rootgfid,
-                                     16, XATTR_CREATE);
-                if (ret) {
+                size = sys_lsetxattr (dir_data->data, "trusted.gfid", rootgfid,
+                                      16, XATTR_CREATE);
+                if (size) {
                         gf_log (this->name, GF_LOG_ERROR,
                                 "%s: failed to set gfid (%s)",
                                 dir_data->data, strerror (errno));
+                        ret = -1;
                         goto out;
                 }
         }
 
-        op_ret = sys_lgetxattr (dir_data->data, "system.posix_acl_access",
-                                NULL, 0);
-        if ((op_ret < 0) && (errno == ENOTSUP))
+        size = sys_lgetxattr (dir_data->data, "system.posix_acl_access",
+                              NULL, 0);
+        if ((size < 0) && (errno == ENOTSUP))
                 gf_log (this->name, GF_LOG_WARNING,
                         "Posix access control list is not supported.");