From 61e738db03b4592c36891ac0f17b321eb7692141 Mon Sep 17 00:00:00 2001 From: Mohit Agrawal Date: Fri, 3 Nov 2017 09:55:53 +0530 Subject: posix: Fix coverity issues in several posix functions Fixes issues 528, 763, 778, 792, 793, 86, 28, 29, 30, 39, 42, 769, 783, 794, 795 from the report at [1]. [1]: https://download.gluster.org/pub/gluster/glusterfs/static-analysis/master/glusterfs-coverity/2017-10-30-9aa574a5/html/ Note: Apart from coverity resolve other issues in posix_get(f)xattr more cleaner way. BUG: 789278 Change-Id: If0737492198481ad7a8d75a3801c862fd61b8c6e Signed-off-by: Mohit Agrawal --- xlators/storage/posix/src/posix.c | 170 ++++++++++++++++++++++++-------------- xlators/storage/posix/src/posix.h | 3 + 2 files changed, 110 insertions(+), 63 deletions(-) (limited to 'xlators/storage') diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c index 672ee49334a..9c09112f9d3 100644 --- a/xlators/storage/posix/src/posix.c +++ b/xlators/storage/posix/src/posix.c @@ -4506,7 +4506,8 @@ posix_get_ancestry_non_directory (xlator_t *this, inode_t *leaf_inode, } while (remaining_size > 0) { - strncpy (key, list + list_offset, sizeof(key)); + strncpy (key, list + list_offset, sizeof(key)-1); + key[sizeof(key)-1] = '\0'; if (strncmp (key, PGFID_XATTR_KEY_PREFIX, strlen (PGFID_XATTR_KEY_PREFIX)) != 0) goto next; @@ -4525,13 +4526,15 @@ posix_get_ancestry_non_directory (xlator_t *this, inode_t *leaf_inode, nlink_samepgfid = ntoh32 (nlink_samepgfid); strncpy (pgfidstr, key + strlen(PGFID_XATTR_KEY_PREFIX), - sizeof(pgfidstr)); + sizeof(pgfidstr)-1); + pgfidstr[sizeof(pgfidstr)-1] = '\0'; gf_uuid_parse (pgfidstr, pgfid); handle_size = POSIX_GFID_HANDLE_SIZE(priv->base_path_length); /* constructing the absolute real path of parent dir */ - strncpy (dirpath, priv->base_path, sizeof(dirpath)); + strncpy (dirpath, priv->base_path, sizeof(dirpath)-1); + dirpath[sizeof(dirpath)-1] = '\0'; pathlen = PATH_MAX + 1 - priv->base_path_length; op_ret = posix_make_ancestryfromgfid (this, @@ -4621,14 +4624,13 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, int ret = -1; char *path = NULL; char *rpath = NULL; - char *dyn_rpath = NULL; ssize_t size = 0; char *list = NULL; int32_t list_offset = 0; size_t remaining_size = 0; - char host_buf[1024] = {0,}; - char keybuffer[4096] = {0,}; - char value_buf[8192] = {0,}; + char *host_buf = NULL; + char *keybuffer = NULL; + char *value_buf = NULL; gf_boolean_t have_val = _gf_false; DECLARE_OLD_FS_ID_VAR; @@ -4646,7 +4648,6 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, ret = posix_handle_georep_xattrs (frame, name, &op_errno, _gf_true); if (ret == -1) { op_ret = -1; - /* errno should be set from the above function*/ goto out; } @@ -4678,11 +4679,11 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, if (loc->inode && name && GF_POSIX_ACL_REQUEST (name)) { ret = posix_pacl_get (real_path, name, &value); if (ret || !value) { + op_errno = errno; gf_msg (this->name, GF_LOG_WARNING, errno, P_MSG_ACL_FAILED, "could not get acl (%s) for" "%s", name, real_path); op_ret = -1; - op_errno = errno; goto out; } @@ -4693,7 +4694,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, P_MSG_ACL_FAILED, "could not set acl (%s) for" "%s in dictionary", name, real_path); op_ret = -1; - op_errno = errno; + op_errno = ENOMEM; goto out; } @@ -4729,16 +4730,22 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, if (loc->inode && name && !strcmp (name, GLUSTERFS_OPEN_FD_COUNT)) { if (!fd_list_empty (loc->inode)) { ret = dict_set_uint32 (dict, (char *)name, 1); - if (ret < 0) + if (ret < 0) { gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_DICT_SET_FAILED, "Failed to set " "dictionary value for %s", name); + op_errno = ENOMEM; + goto out; + } } else { ret = dict_set_uint32 (dict, (char *)name, 0); - if (ret < 0) + if (ret < 0) { gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_DICT_SET_FAILED, "Failed to set " "dictionary value for %s", name); + op_errno = ENOMEM; + goto out; + } } goto done; } @@ -4748,26 +4755,24 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, else rpath = real_path; - (void) snprintf (host_buf, sizeof(host_buf), - "", priv->base_path, - ((priv->node_uuid_pathinfo - && !gf_uuid_is_null(priv->glusterd_uuid)) - ? uuid_utoa (priv->glusterd_uuid) - : priv->hostname), - rpath); - - dyn_rpath = gf_strdup (host_buf); - if (!dyn_rpath) { - ret = -1; - goto done; + size = gf_asprintf (&host_buf, "", + priv->base_path, + ((priv->node_uuid_pathinfo && + !gf_uuid_is_null(priv->glusterd_uuid)) + ? uuid_utoa (priv->glusterd_uuid) + : priv->hostname), rpath); + if (size < 0) { + op_errno = ENOMEM; + goto out; } - size = strlen (dyn_rpath) + 1; - ret = dict_set_dynstr (dict, (char *)name, dyn_rpath); + ret = dict_set_dynstr (dict, (char *)name, host_buf); if (ret < 0) { gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_DICT_SET_FAILED, "could not set value" - " (%s) in dictionary", dyn_rpath); - GF_FREE (dyn_rpath); + " (%s) in dictionary", host_buf); + GF_FREE (host_buf); + op_errno = ENOMEM; + goto out; } goto done; @@ -4776,23 +4781,19 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, if (loc->inode && name && (strcmp (name, GF_XATTR_NODE_UUID_KEY) == 0) && !gf_uuid_is_null (priv->glusterd_uuid)) { - (void) snprintf (host_buf, sizeof(host_buf), "%s", - uuid_utoa (priv->glusterd_uuid)); - - dyn_rpath = gf_strdup (host_buf); - if (!dyn_rpath) { + size = gf_asprintf (&host_buf, "%s", + uuid_utoa (priv->glusterd_uuid)); + if (size == -1) { op_errno = ENOMEM; goto out; } - - size = strlen (dyn_rpath) + 1; ret = dict_set_dynstr (dict, GF_XATTR_NODE_UUID_KEY, - dyn_rpath); + host_buf); if (ret < 0) { gf_msg (this->name, GF_LOG_WARNING, -ret, P_MSG_DICT_SET_FAILED, "could not set value" - "(%s) in dictionary", dyn_rpath); - GF_FREE (dyn_rpath); + "(%s) in dictionary", host_buf); + GF_FREE (host_buf); op_errno = -ret; goto out; } @@ -4850,15 +4851,15 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, op_errno = ENODATA; goto out; } - + size = op_ret; op_ret = dict_set_dynstr (dict, GET_ANCESTRY_PATH_KEY, path); if (op_ret < 0) { gf_msg (this->name, GF_LOG_WARNING, -op_ret, P_MSG_GET_KEY_VALUE_FAILED, "could not get " "value for key (%s)", GET_ANCESTRY_PATH_KEY); GF_FREE (path); - op_errno = -op_ret; - op_ret = -1; + op_errno = ENOMEM; + goto out; } goto done; @@ -4870,15 +4871,22 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, op_ret = posix_get_objectsignature (real_path, dict); if (op_ret < 0) { op_errno = -op_ret; - op_ret = -1; + goto out; } goto done; } + /* here allocate value_buf of 8192 bytes to avoid one extra getxattr + call,If buffer size is small to hold the xattr result then it will + allocate a new buffer value of required size and call getxattr again + */ + + value_buf = alloca (XATTR_VAL_BUF_SIZE); if (name) { - strncpy (keybuffer, name, sizeof(keybuffer)); - char *key = keybuffer; + char *key = (char *)name; + + keybuffer = key; #if defined(GF_DARWIN_HOST_OS_DISABLED) if (priv->xattr_user_namespace == XATTR_STRIP) { if (strncmp(key, "user.",5) == 0) { @@ -4889,9 +4897,9 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, } } #endif - memset (value_buf, '\0', sizeof(value_buf)); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); size = sys_lgetxattr (real_path, key, value_buf, - sizeof (value_buf) - 1); + XATTR_VAL_BUF_SIZE-1); if (size >= 0) { have_val = _gf_true; } else { @@ -4925,7 +4933,7 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, "getxattr failed on %s: %s ", real_path, key); } - goto done; + goto out; } } value = GF_CALLOC (size + 1, sizeof(char), gf_posix_mt_char); @@ -4964,8 +4972,8 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, } have_val = _gf_false; - memset (value_buf, '\0', sizeof(value_buf)); - size = sys_llistxattr (real_path, value_buf, sizeof (value_buf) - 1); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); + size = sys_llistxattr (real_path, value_buf, XATTR_VAL_BUF_SIZE-1); if (size > 0) { have_val = _gf_true; } else { @@ -5012,8 +5020,10 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, } remaining_size = size; list_offset = 0; + keybuffer = alloca (XATTR_KEY_BUF_SIZE); while (remaining_size > 0) { - strncpy (keybuffer, list + list_offset, sizeof(keybuffer)); + strncpy (keybuffer, list + list_offset, XATTR_KEY_BUF_SIZE-1); + keybuffer[XATTR_KEY_BUF_SIZE-1] = '\0'; ret = posix_handle_georep_xattrs (frame, keybuffer, NULL, _gf_false); @@ -5024,10 +5034,10 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, goto ignore; } - memset (value_buf, '\0', sizeof(value_buf)); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); have_val = _gf_false; size = sys_lgetxattr (real_path, keybuffer, value_buf, - sizeof (value_buf) - 1); + XATTR_VAL_BUF_SIZE-1); if (size >= 0) { have_val = _gf_true; } else { @@ -5041,12 +5051,11 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, NULL, 0); } if (size == -1) { - op_ret = -1; op_errno = errno; gf_msg (this->name, GF_LOG_ERROR, errno, P_MSG_XATTR_FAILED, "getxattr failed on" " %s: key = %s ", real_path, keybuffer); - break; + goto out; } } value = GF_CALLOC (size + 1, sizeof(char), @@ -5060,13 +5069,12 @@ posix_getxattr (call_frame_t *frame, xlator_t *this, } else { size = sys_lgetxattr (real_path, keybuffer, value, size); if (size == -1) { - op_ret = -1; op_errno = errno; gf_msg (this->name, GF_LOG_ERROR, errno, P_MSG_XATTR_FAILED, "getxattr failed on" " %s: key = %s ", real_path, keybuffer); GF_FREE (value); - break; + goto out; } } value [size] = '\0'; @@ -5131,7 +5139,7 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, dict_t * dict = NULL; int ret = -1; char key[4096] = {0,}; - char value_buf[8192] = {0,}; + char *value_buf = NULL; gf_boolean_t have_val = _gf_false; DECLARE_OLD_FS_ID_VAR; @@ -5169,6 +5177,7 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_DICT_SET_FAILED, "Failed to set " "dictionary value for %s", name); + goto out; } goto done; } @@ -5182,11 +5191,19 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, op_errno = -op_ret; op_ret = -1; size = -1; + goto out; } goto done; } + /* here allocate value_buf of 8192 bytes to avoid one extra getxattr + call,If buffer size is small to hold the xattr result then it will + allocate a new buffer value of required size and call getxattr again + */ + value_buf = alloca (XATTR_VAL_BUF_SIZE); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); + if (name) { strncpy (key, name, sizeof(key)); #ifdef GF_DARWIN_HOST_OS @@ -5199,7 +5216,8 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, GF_FREE (newkey); } #endif - size = sys_fgetxattr (_fd, key, value_buf, sizeof(value_buf) - 1); + size = sys_fgetxattr (_fd, key, value_buf, + XATTR_VAL_BUF_SIZE-1); if (size >= 0) { have_val = _gf_true; } else { @@ -5260,8 +5278,8 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, goto done; } - memset (value_buf, '\0', sizeof(value_buf)); - size = sys_flistxattr (_fd, value_buf, sizeof (value_buf) - 1); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); + size = sys_flistxattr (_fd, value_buf, XATTR_VAL_BUF_SIZE-1); if (size > 0) { have_val = _gf_true; } else { @@ -5309,9 +5327,10 @@ posix_fgetxattr (call_frame_t *frame, xlator_t *this, break; strncpy (key, list + list_offset, sizeof(key)); - memset (value_buf, '\0', sizeof(value_buf)); + memset (value_buf, '\0', XATTR_VAL_BUF_SIZE); have_val = _gf_false; - size = sys_fgetxattr (_fd, key, value_buf, sizeof (value_buf) - 1); + size = sys_fgetxattr (_fd, key, value_buf, + XATTR_VAL_BUF_SIZE-1); if (size >= 0) { have_val = _gf_true; } else { @@ -5433,7 +5452,14 @@ posix_fsetxattr (call_frame_t *frame, xlator_t *this, } _fd = pfd->fd; - posix_fdstat (this, pfd->fd, &stbuf); + ret = posix_fdstat (this, pfd->fd, &stbuf); + if (ret == -1) { + op_errno = errno; + gf_msg (this->name, GF_LOG_ERROR, op_errno, + P_MSG_FSTAT_FAILED, "fsetxattr (fstat)" + "failed on fd=%p", fd); + goto out; + } dict_del (dict, GFID_XATTR_KEY); dict_del (dict, GF_XATTR_VOL_ID_KEY); @@ -7890,6 +7916,19 @@ init (xlator_t *this) GF_OPTION_INIT ("batch-fsync-delay-usec", _private->batch_fsync_delay_usec, uint32, out); out: + if (ret) { + if (_private) { + GF_FREE (_private->base_path); + + GF_FREE (_private->hostname); + + GF_FREE (_private->trash_path); + + GF_FREE (_private); + } + + this->private = NULL; + } return ret; } @@ -7903,7 +7942,12 @@ fini (xlator_t *this) /*unlock brick dir*/ if (priv->mount_lock) (void) sys_closedir (priv->mount_lock); + + GF_FREE (priv->base_path); + GF_FREE (priv->hostname); + GF_FREE (priv->trash_path); GF_FREE (priv); + return; } struct xlator_dumpops dumpops = { diff --git a/xlators/storage/posix/src/posix.h b/xlators/storage/posix/src/posix.h index 1e19311cb03..dc8ac0106ab 100644 --- a/xlators/storage/posix/src/posix.h +++ b/xlators/storage/posix/src/posix.h @@ -47,6 +47,9 @@ #define VECTOR_SIZE 64 * 1024 /* vector size 64KB*/ #define MAX_NO_VECT 1024 +#define XATTR_KEY_BUF_SIZE 4096 +#define XATTR_VAL_BUF_SIZE 8192 + #define ACL_BUFFER_MAX 4096 /* size of character buffer */ #define DHT_LINKTO "trusted.glusterfs.dht.linkto" -- cgit