From d3b1456c52f7dc4f21cdae2855092fda6b96af4a Mon Sep 17 00:00:00 2001 From: Amar Tumballi Date: Tue, 28 Aug 2018 00:01:26 +0530 Subject: clang-scan: fix multiple issues * Buffer overflow issue in glusterfsd * Null argument passed to function expecting non-null (event-epoll) * Make sure the op_ret value is set in macro (posix) Updates: bz#1622665 Change-Id: I32b378fc40a5e3ee800c0dfbc13335d44c9db9ac Signed-off-by: Amar Tumballi --- glusterfsd/src/glusterfsd.c | 17 +++++++++++----- libglusterfs/src/event-epoll.c | 25 ++++++++++++++++++++++++ xlators/protocol/server/src/server-helpers.c | 2 +- xlators/protocol/server/src/server-rpc-fops.c | 9 +++++++++ xlators/protocol/server/src/server-rpc-fops_v2.c | 9 +++++++++ xlators/protocol/server/src/server.c | 2 +- xlators/storage/posix/src/posix-entry-ops.c | 2 +- xlators/storage/posix/src/posix-helpers.c | 4 ++-- xlators/storage/posix/src/posix-inode-fd-ops.c | 17 +++++++++++----- xlators/storage/posix/src/posix-inode-handle.h | 4 ++++ 10 files changed, 76 insertions(+), 15 deletions(-) diff --git a/glusterfsd/src/glusterfsd.c b/glusterfsd/src/glusterfsd.c index c3fea207b6e..12f561eb689 100644 --- a/glusterfsd/src/glusterfsd.c +++ b/glusterfsd/src/glusterfsd.c @@ -2720,11 +2720,18 @@ main (int argc, char *argv[]) command line options. */ { int i = 0; - strcpy (cmdlinestr, argv[0]); - for (i = 1; i < argc; i++) { - strcat (cmdlinestr, " "); - strncat (cmdlinestr, argv[i], - (sizeof (cmdlinestr) - 1)); + int pos = 0; + int len = snprintf (cmdlinestr, sizeof (cmdlinestr), "%s", argv[0]); + for (i = 1; (i < argc) && (len > 0); i++) { + pos += len; + len = snprintf (cmdlinestr + pos, sizeof (cmdlinestr) - pos, " %s", + argv[i]); + if ((len <= 0) || (len >= (sizeof (cmdlinestr) - pos))) { + gf_msg ("glusterfs", GF_LOG_ERROR, 0, glusterfsd_msg_29, + "failed to create command line string"); + ret = -1; + goto out; + } } gf_msg (argv[0], GF_LOG_INFO, 0, glusterfsd_msg_30, "Started running %s version %s (args: %s)", diff --git a/libglusterfs/src/event-epoll.c b/libglusterfs/src/event-epoll.c index 8ff66a8445f..b144b77a11c 100644 --- a/libglusterfs/src/event-epoll.c +++ b/libglusterfs/src/event-epoll.c @@ -349,6 +349,11 @@ event_register_epoll (struct event_pool *event_pool, int fd, } slot = event_slot_get (event_pool, idx); + if (!slot) { + gf_msg ("epoll", GF_LOG_ERROR, 0, LG_MSG_SLOT_NOT_FOUND, + "could not find slot for fd=%d idx=%d", fd, idx); + return -1; + } assert (slot->fd == fd); @@ -413,6 +418,11 @@ event_unregister_epoll_common (struct event_pool *event_pool, int fd, goto out; slot = event_slot_get (event_pool, idx); + if (!slot) { + gf_msg ("epoll", GF_LOG_ERROR, 0, LG_MSG_SLOT_NOT_FOUND, + "could not find slot for fd=%d idx=%d", fd, idx); + return -1; + } assert (slot->fd == fd); @@ -477,6 +487,11 @@ event_select_on_epoll (struct event_pool *event_pool, int fd, int idx, GF_VALIDATE_OR_GOTO ("event", event_pool, out); slot = event_slot_get (event_pool, idx); + if (!slot) { + gf_msg ("epoll", GF_LOG_ERROR, 0, LG_MSG_SLOT_NOT_FOUND, + "could not find slot for fd=%d idx=%d", fd, idx); + return -1; + } assert (slot->fd == fd); @@ -544,6 +559,11 @@ event_dispatch_epoll_handler (struct event_pool *event_pool, gen = ev_data->gen; slot = event_slot_get (event_pool, idx); + if (!slot) { + gf_msg ("epoll", GF_LOG_ERROR, 0, LG_MSG_SLOT_NOT_FOUND, + "could not find slot for idx=%d", idx); + return -1; + } LOCK (&slot->lock); { @@ -902,6 +922,11 @@ event_handled_epoll (struct event_pool *event_pool, int fd, int idx, int gen) int ret = 0; slot = event_slot_get (event_pool, idx); + if (!slot) { + gf_msg ("epoll", GF_LOG_ERROR, 0, LG_MSG_SLOT_NOT_FOUND, + "could not find slot for fd=%d idx=%d", fd, idx); + return -1; + } assert (slot->fd == fd); diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c index f6fb32ed140..ce2097765b1 100644 --- a/xlators/protocol/server/src/server-helpers.c +++ b/xlators/protocol/server/src/server-helpers.c @@ -331,7 +331,7 @@ server_connection_cleanup (xlator_t *this, client_t *client, int cd_ret = 0; int ret = 0; - GF_VALIDATE_OR_GOTO (this->name, this, out); + GF_VALIDATE_OR_GOTO ("server", this, out); GF_VALIDATE_OR_GOTO (this->name, client, out); GF_VALIDATE_OR_GOTO (this->name, flags, out); diff --git a/xlators/protocol/server/src/server-rpc-fops.c b/xlators/protocol/server/src/server-rpc-fops.c index 915e166223c..c5015befa7e 100644 --- a/xlators/protocol/server/src/server-rpc-fops.c +++ b/xlators/protocol/server/src/server-rpc-fops.c @@ -2201,6 +2201,15 @@ server_compound_cbk (call_frame_t *frame, void *cookie, xlator_t *this, STACK_ERR_XL_NAME (frame->root)); } + /* TODO: I assume a single 10MB payload is large, if not, we need to + agree to valid payload */ + if ((args_cbk->fop_length <= 0) || + ((args_cbk->fop_length > (10 * 1024 * 1024)))) { + op_ret = -1; + op_errno = EINVAL; + goto out; + } + rsp.compound_rsp_array.compound_rsp_array_val = GF_CALLOC (args_cbk->fop_length, sizeof (compound_rsp), diff --git a/xlators/protocol/server/src/server-rpc-fops_v2.c b/xlators/protocol/server/src/server-rpc-fops_v2.c index 09d404f2d86..64ca0bbf65b 100644 --- a/xlators/protocol/server/src/server-rpc-fops_v2.c +++ b/xlators/protocol/server/src/server-rpc-fops_v2.c @@ -5830,6 +5830,15 @@ server4_compound_cbk (call_frame_t *frame, void *cookie, xlator_t *this, STACK_ERR_XL_NAME (frame->root)); } + /* TODO: I assume a single 10MB payload is large, if not, we need to + agree to valid payload */ + if ((args_cbk->fop_length <= 0) || + ((args_cbk->fop_length > (10 * 1024 * 1024)))) { + op_ret = -1; + op_errno = EINVAL; + goto out; + } + rsp.compound_rsp_array.compound_rsp_array_val = GF_CALLOC (args_cbk->fop_length, sizeof (compound_rsp_v2), diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c index 4cf4b4aeac1..c95a541cbc2 100644 --- a/xlators/protocol/server/src/server.c +++ b/xlators/protocol/server/src/server.c @@ -187,7 +187,7 @@ server_priv_to_dict (xlator_t *this, dict_t *dict, char *brickname) pthread_mutex_lock (&conf->mutex); { list_for_each_entry (xprt, &conf->xprt_list, list) { - if ((xprt) && (xprt->xl_private) && + if ((xprt->xl_private) && (xprt->xl_private->bound_xl) && (xprt->xl_private->bound_xl->name) && (brickname) && (!strcmp (brickname, diff --git a/xlators/storage/posix/src/posix-entry-ops.c b/xlators/storage/posix/src/posix-entry-ops.c index 11ce9d69540..34dff4bd726 100644 --- a/xlators/storage/posix/src/posix-entry-ops.c +++ b/xlators/storage/posix/src/posix-entry-ops.c @@ -1725,7 +1725,7 @@ posix_rename (call_frame_t *frame, xlator_t *this, } if ((xdata) && (dict_get (xdata, GET_LINK_COUNT)) - && (real_newpath) && (was_present)) { + && (real_newpath) && (was_present) && ctx_new) { pthread_mutex_lock (&ctx_new->pgfid_lock); locked = _gf_true; get_link_count = _gf_true; diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c index d521d80e8aa..99be6366e11 100644 --- a/xlators/storage/posix/src/posix-helpers.c +++ b/xlators/storage/posix/src/posix-helpers.c @@ -3390,7 +3390,7 @@ posix_cs_maintenance (xlator_t *this, fd_t *fd, loc_t *loc, int *pfd, } else { if (!loc->inode) { ret = 0; - goto unlock; + goto out; } LOCK (&loc->inode->lock); @@ -3450,6 +3450,6 @@ unlock: UNLOCK (&fd->inode->lock); else UNLOCK (&loc->inode->lock); - +out: return ret; } diff --git a/xlators/storage/posix/src/posix-inode-fd-ops.c b/xlators/storage/posix/src/posix-inode-fd-ops.c index 795f19a1515..64fa5ea7a82 100644 --- a/xlators/storage/posix/src/posix-inode-fd-ops.c +++ b/xlators/storage/posix/src/posix-inode-fd-ops.c @@ -1257,13 +1257,14 @@ posix_readlink (call_frame_t *frame, xlator_t *this, { char * dest = NULL; int32_t op_ret = -1; - int32_t op_errno = 0; + int32_t op_errno = EINVAL; char * real_path = NULL; struct iatt stbuf = {0,}; DECLARE_OLD_FS_ID_VAR; VALIDATE_OR_GOTO (frame, out); + VALIDATE_OR_GOTO (loc, out); SET_FS_ID (frame->root->uid, frame->root->gid); @@ -3993,12 +3994,15 @@ 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; + int op_ret = -1; + int op_errno = EINVAL; dict_t *xdata_rsp = NULL; + VALIDATE_OR_GOTO (loc, out); + op_ret = posix_common_removexattr (frame, loc, NULL, name, xdata, &op_errno, &xdata_rsp); +out: STACK_UNWIND_STRICT (removexattr, frame, op_ret, op_errno, xdata_rsp); if (xdata_rsp) @@ -4011,12 +4015,15 @@ int32_t posix_fremovexattr (call_frame_t *frame, xlator_t *this, fd_t *fd, const char *name, dict_t *xdata) { - int32_t op_ret = 0; - int32_t op_errno = 0; + int32_t op_ret = -1; + int32_t op_errno = EINVAL; dict_t *xdata_rsp = NULL; + VALIDATE_OR_GOTO (fd, out); + op_ret = posix_common_removexattr (frame, NULL, fd, name, xdata, &op_errno, &xdata_rsp); +out: STACK_UNWIND_STRICT (fremovexattr, frame, op_ret, op_errno, xdata_rsp); if (xdata_rsp) diff --git a/xlators/storage/posix/src/posix-inode-handle.h b/xlators/storage/posix/src/posix-inode-handle.h index 33d908fa3de..cb315424dd0 100644 --- a/xlators/storage/posix/src/posix-inode-handle.h +++ b/xlators/storage/posix/src/posix-inode-handle.h @@ -54,14 +54,18 @@ var = NULL; \ } while (0) +/* TODO: it is not a good idea to change a variable which + is not passed to the macro.. Fix it later */ #define MAKE_INODE_HANDLE(rpath, this, loc, iatt_p) do { \ if (!this->private) { \ + op_ret = -1; \ gf_msg ("make_inode_handle", GF_LOG_ERROR, 0, \ P_MSG_INODE_HANDLE_CREATE, \ "private is NULL, fini is already called"); \ break; \ } \ if (gf_uuid_is_null (loc->gfid)) { \ + op_ret = -1; \ gf_msg (this->name, GF_LOG_ERROR, 0, \ P_MSG_INODE_HANDLE_CREATE, \ "null gfid for path %s", (loc)->path); \ -- cgit