From d5e317718f4137431c3996ac5f38e9226620e760 Mon Sep 17 00:00:00 2001 From: "Kaleb S. KEITHLEY" Date: Mon, 28 Apr 2014 14:25:09 -0400 Subject: core: fix Ubuntu code audit (cppcheck) results These block inclusion in Ubuntu Main repo. AFAICT these are false positives: [rpc/rpc-transport/rdma/src/rdma.c:3074]: (error) Address of local auto-variable assigned to a function parameter. [xlators/features/marker/utils/src/gsyncd.c:99]: (error) Memory leak: str [xlators/features/marker/utils/src/gsyncd.c:354]: (error) Memory leak: argv [xlators/nfs/server/src/nlm4.c:1176]: (error) Possible null pointer dereference: fde The remainder are fixed with this change-set: [api/src/glfs-fops.c:700]: (error) Possible null pointer dereference: gio [api/src/glfs-fops.c:702]: (error) Possible null pointer dereference: frame [xlators/cluster/afr/src/afr-inode-write.c:375]: (error) Possible null pointer dereference: frame [xlators/cluster/afr/src/afr-self-heal-common.c:1522]: (error) Possible null pointer dereference: local [xlators/cluster/dht/src/dht-rebalance.c:1574]: (error) Possible null pointer dereference: ctx [xlators/cluster/stripe/src/stripe.c:4407]: (error) Possible null pointer dereference: local [xlators/mgmt/glusterd/src/glusterd-mountbroker.c:675]: (error) Possible null pointer dereference: cookieswitch [xlators/mgmt/glusterd/src/glusterd-mountbroker.c:677]: (error) Possible null pointer dereference: cookieswitch [xlators/mgmt/glusterd/src/glusterd-replace-brick.c:924]: (error) Resource leak: file [xlators/mgmt/glusterd/src/glusterd-replace-brick.c:1008]: (error) Resource leak: file [xlators/mgmt/glusterd/src/glusterd-sm.c:248]: (error) Possible null pointer dereference: new_ev_ctx [xlators/mgmt/glusterd/src/glusterd-store.c:1250]: (error) Possible null pointer dereference: handle [xlators/mgmt/glusterd/src/glusterd-utils.c:4272]: (error) Possible null pointer dereference: this [xlators/mgmt/glusterd/src/glusterd-utils.c:5113]: (error) Possible null pointer dereference: this [xlators/mount/fuse/src/fuse-bridge.c:4432]: (error) Uninitialized variable: finh [xlators/mount/fuse/src/fuse-bridge.c:2927]: (error) Possible null pointer dereference: state [xlators/mount/fuse/src/fuse-bridge.c:3226]: (error) Possible null pointer dereference: state [xlators/storage/bd_map/src/bd_map.c:1504]: (error) Possible null pointer dereference: bd_fd [xlators/storage/bd_map/src/bd_map.c:1728]: (error) Possible null pointer dereference: n_entry [xlators/storage/bd_map/src/bd_map.c:1741]: (error) Possible null pointer dereference: n_entry [xlators/performance/quick-read/src/quick-read.c:585]: (error) Possible null pointer dereference: iobuf rerunning cppcheck --force afterwards: Test code, don't care: [extras/test/test-ffop.c:27]: (error) Buffer overrun possible for long command line arguments. False positive after fix [xlators/cluster/stripe/src/stripe.c:4407]: (error) Possible null pointer dereference: local Still false positive: [xlators/features/marker/utils/src/gsyncd.c:354]: (error) Memory leak: argv [xlators/nfs/server/src/nlm4.c:1176]: (error) Possible null pointer dereference: fde Not built, don't care: [xlators/cluster/ha/src/ha.c:2699]: (error) Possible null pointer dereference: priv Change-Id: I1fb849e9c042d3a3701cb05121d413e58e73d505 BUG: 1086460 Signed-off-by: Kaleb S. KEITHLEY Reviewed-on: http://review.gluster.org/7583 Reviewed-by: Vijay Bellur Tested-by: Gluster Build System --- api/src/glfs-fops.c | 6 ++++-- xlators/cluster/afr/src/afr-self-heal-common.c | 4 +++- xlators/cluster/afr/src/afr.h | 10 ++++++---- xlators/cluster/dht/src/dht-common.c | 4 ---- xlators/cluster/dht/src/dht-rebalance.c | 2 +- xlators/cluster/stripe/src/stripe.c | 13 +++++++++---- xlators/features/marker/utils/src/gsyncd.c | 2 +- xlators/mgmt/glusterd/src/glusterd-mountbroker.c | 2 +- xlators/mgmt/glusterd/src/glusterd-replace-brick.c | 6 ++++-- xlators/mgmt/glusterd/src/glusterd-sm.c | 3 ++- xlators/mgmt/glusterd/src/glusterd-store.c | 4 ++-- xlators/mgmt/glusterd/src/glusterd-utils.c | 12 ++++++------ xlators/mount/fuse/src/fuse-bridge.c | 9 +++++---- xlators/nfs/server/src/nfs-common.c | 11 ++++------- xlators/performance/quick-read/src/quick-read.c | 13 +++---------- xlators/storage/bd_map/src/bd_map.c | 9 ++++----- 16 files changed, 55 insertions(+), 55 deletions(-) diff --git a/api/src/glfs-fops.c b/api/src/glfs-fops.c index bda7e9e467e..61843c8a857 100644 --- a/api/src/glfs-fops.c +++ b/api/src/glfs-fops.c @@ -697,9 +697,11 @@ glfs_preadv_async (struct glfs_fd *glfd, const struct iovec *iovec, int count, out: if (ret) { - GF_FREE (gio->iov); + if (gio) + GF_FREE (gio->iov); GF_FREE (gio); - STACK_DESTROY (frame->root); + if (frame) + STACK_DESTROY (frame->root); glfs_subvol_done (fs, subvol); } diff --git a/xlators/cluster/afr/src/afr-self-heal-common.c b/xlators/cluster/afr/src/afr-self-heal-common.c index 600991d5058..c6ce7dc5f39 100644 --- a/xlators/cluster/afr/src/afr-self-heal-common.c +++ b/xlators/cluster/afr/src/afr-self-heal-common.c @@ -1508,6 +1508,8 @@ afr_sh_remove_entry_cbk (call_frame_t *frame, xlator_t *this, int child, afr_self_heal_t *sh = NULL; local = frame->local; + GF_ASSERT (local); + sh = &local->self_heal; GF_ASSERT (sh->post_remove_call); @@ -1567,7 +1569,7 @@ afr_sh_call_entry_expunge_remove (call_frame_t *frame, xlator_t *this, return; out: gf_log (this->name, GF_LOG_ERROR, "Expunge of %s failed, reason: %s", - local->loc.path, strerror (op_errno)); + local ? local->loc.path : "" , strerror (op_errno)); expunge_done (frame, this, child_index, -1, op_errno); } diff --git a/xlators/cluster/afr/src/afr.h b/xlators/cluster/afr/src/afr.h index 978339017a2..84e8907f993 100644 --- a/xlators/cluster/afr/src/afr.h +++ b/xlators/cluster/afr/src/afr.h @@ -892,10 +892,12 @@ afr_launch_openfd_self_heal (call_frame_t *frame, xlator_t *this, fd_t *fd); do { \ afr_local_t *__local = NULL; \ xlator_t *__this = NULL; \ - __local = frame->local; \ - __this = frame->this; \ - frame->local = NULL; \ - STACK_DESTROY (frame->root); \ + if (frame) { \ + __local = frame->local; \ + __this = frame->this; \ + frame->local = NULL; \ + STACK_DESTROY (frame->root); \ + } \ if (__local) { \ afr_local_cleanup (__local, __this); \ mem_put (__local); \ diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c index 29bfd21606b..0234bbbd409 100644 --- a/xlators/cluster/dht/src/dht-common.c +++ b/xlators/cluster/dht/src/dht-common.c @@ -2062,10 +2062,8 @@ dht_getxattr_get_real_filename_cbk (call_frame_t *frame, void *cookie, { int this_call_cnt = 0; dht_local_t *local = NULL; - dht_conf_t *conf = NULL; - conf = this->private; local = frame->local; if (op_ret != -1) { @@ -2092,7 +2090,6 @@ int dht_getxattr_get_real_filename (call_frame_t *frame, xlator_t *this, loc_t *loc, const char *key, dict_t *xdata) { - dht_conf_t *conf = NULL; dht_local_t *local = NULL; int i = 0; dht_layout_t *layout = NULL; @@ -2100,7 +2097,6 @@ dht_getxattr_get_real_filename (call_frame_t *frame, xlator_t *this, xlator_t *subvol = NULL; - conf = this->private; local = frame->local; layout = local->layout; diff --git a/xlators/cluster/dht/src/dht-rebalance.c b/xlators/cluster/dht/src/dht-rebalance.c index d212f6eae25..c181bebd131 100644 --- a/xlators/cluster/dht/src/dht-rebalance.c +++ b/xlators/cluster/dht/src/dht-rebalance.c @@ -1571,7 +1571,7 @@ out: { status = dict_new (); gf_defrag_status_get (defrag, status); - if (ctx->notify) + if (ctx && ctx->notify) ctx->notify (GF_EN_DEFRAG_STATUS, status); if (status) dict_unref (status); diff --git a/xlators/cluster/stripe/src/stripe.c b/xlators/cluster/stripe/src/stripe.c index b3e87d31b5a..bdea45f3ed6 100644 --- a/xlators/cluster/stripe/src/stripe.c +++ b/xlators/cluster/stripe/src/stripe.c @@ -4402,10 +4402,15 @@ unlock: out: if (!count) { /* all entries are directories */ - frame->local = NULL; - STRIPE_STACK_UNWIND (readdir, frame, local->op_ret, - local->op_errno, &local->entries, NULL); - gf_dirent_free (&local->entries); + if (frame) + frame->local = NULL; + STRIPE_STACK_UNWIND (readdir, frame, + local ? local->op_ret : -1, + local ? local->op_errno : EINVAL, + local ? &local->entries : NULL, + NULL); + if (local) + gf_dirent_free (&local->entries); stripe_local_wipe (local); mem_put (local); } diff --git a/xlators/features/marker/utils/src/gsyncd.c b/xlators/features/marker/utils/src/gsyncd.c index 9c4a5bdffb3..1f9eb2fa958 100644 --- a/xlators/features/marker/utils/src/gsyncd.c +++ b/xlators/features/marker/utils/src/gsyncd.c @@ -70,7 +70,7 @@ str2argv (char *str, char ***argv) int ret = 0; assert (str); - str = strdup (str); + str = gf_strdup (str); if (!str) return -1; diff --git a/xlators/mgmt/glusterd/src/glusterd-mountbroker.c b/xlators/mgmt/glusterd/src/glusterd-mountbroker.c index 0d67d130360..c96804a80b4 100644 --- a/xlators/mgmt/glusterd/src/glusterd-mountbroker.c +++ b/xlators/mgmt/glusterd/src/glusterd-mountbroker.c @@ -671,7 +671,7 @@ glusterd_do_mount (char *label, dict_t *argdict, char **path, int *op_errno) ret = -1; gf_log ("", GF_LOG_WARNING, "unsuccessful mount request (%s)", strerror (*op_errno)); - if (mtptemp) { + if (mtptemp && cookieswitch) { *cookieswitch = '/'; unlink (mtptemp); *cookieswitch = '\0'; diff --git a/xlators/mgmt/glusterd/src/glusterd-replace-brick.c b/xlators/mgmt/glusterd/src/glusterd-replace-brick.c index 5f299a7eff5..da02edd3674 100644 --- a/xlators/mgmt/glusterd/src/glusterd-replace-brick.c +++ b/xlators/mgmt/glusterd/src/glusterd-replace-brick.c @@ -891,7 +891,7 @@ rb_generate_client_volfile (glusterd_volinfo_t *volinfo, "%s", strerror (errno)); goto out; } - close (fd); + sys_close (fd); file = fopen (filename, "w+"); if (!file) { @@ -906,6 +906,7 @@ rb_generate_client_volfile (glusterd_volinfo_t *volinfo, ttype = glusterd_get_trans_type_rb (volinfo->transport_type); if (NULL == ttype){ ret = -1; + fclose (file); goto out; } @@ -970,7 +971,7 @@ rb_generate_dst_brick_volfile (glusterd_volinfo_t *volinfo, "%s", strerror (errno)); goto out; } - close (fd); + sys_close (fd); file = fopen (filename, "w+"); if (!file) { @@ -983,6 +984,7 @@ rb_generate_dst_brick_volfile (glusterd_volinfo_t *volinfo, trans_type = glusterd_get_trans_type_rb (volinfo->transport_type); if (NULL == trans_type){ ret = -1; + fclose (file); goto out; } diff --git a/xlators/mgmt/glusterd/src/glusterd-sm.c b/xlators/mgmt/glusterd/src/glusterd-sm.c index c2bf493004a..8d9c5215fa9 100644 --- a/xlators/mgmt/glusterd/src/glusterd-sm.c +++ b/xlators/mgmt/glusterd/src/glusterd-sm.c @@ -245,7 +245,8 @@ glusterd_ac_reverse_probe_begin (glusterd_friend_sm_event_t *event, void *ctx) out: if (ret) { GF_FREE (new_event); - GF_FREE (new_ev_ctx->hostname); + if (new_ev_ctx) + GF_FREE (new_ev_ctx->hostname); GF_FREE (new_ev_ctx); } gf_log ("", GF_LOG_DEBUG, "returning with %d", ret); diff --git a/xlators/mgmt/glusterd/src/glusterd-store.c b/xlators/mgmt/glusterd/src/glusterd-store.c index 1790c5aec24..023fdc62f65 100644 --- a/xlators/mgmt/glusterd/src/glusterd-store.c +++ b/xlators/mgmt/glusterd/src/glusterd-store.c @@ -1247,10 +1247,10 @@ glusterd_store_global_info (xlator_t *this) ret = gf_store_rename_tmppath (handle); out: - if (ret && (handle->fd > 0)) + if (ret && handle && (handle->fd > 0)) gf_store_unlink_tmppath (handle); - if (handle->fd > 0) { + if (handle && handle->fd > 0) { close (handle->fd); handle->fd = 0; } diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c index c336337e866..6706cb6f36c 100644 --- a/xlators/mgmt/glusterd/src/glusterd-utils.c +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c @@ -4240,14 +4240,14 @@ glusterd_brick_start (glusterd_volinfo_t *volinfo, xlator_t *this = NULL; glusterd_conf_t *conf = NULL; - if ((!brickinfo) || (!volinfo)) - goto out; - this = THIS; GF_ASSERT (this); conf = this->private; GF_ASSERT (conf); + if ((!brickinfo) || (!volinfo)) + goto out; + if (uuid_is_null (brickinfo->uuid)) { ret = glusterd_resolve_brick (brickinfo); if (ret) { @@ -5074,14 +5074,14 @@ glusterd_brick_stop (glusterd_volinfo_t *volinfo, xlator_t *this = NULL; glusterd_conf_t *conf = NULL; - if ((!brickinfo) || (!volinfo)) - goto out; - this = THIS; GF_ASSERT (this); conf = this->private; GF_ASSERT (conf); + if ((!brickinfo) || (!volinfo)) + goto out; + if (uuid_is_null (brickinfo->uuid)) { ret = glusterd_resolve_brick (brickinfo); if (ret) { diff --git a/xlators/mount/fuse/src/fuse-bridge.c b/xlators/mount/fuse/src/fuse-bridge.c index e9c5383e65a..e531970e5c4 100644 --- a/xlators/mount/fuse/src/fuse-bridge.c +++ b/xlators/mount/fuse/src/fuse-bridge.c @@ -2919,6 +2919,8 @@ fuse_setxattr (xlator_t *this, fuse_in_header_t *finh, void *msg) priv = this->private; + GET_STATE (this, finh, state); + #ifdef GF_DARWIN_HOST_OS if (fsi->position) { gf_log ("glusterfs-fuse", GF_LOG_WARNING, @@ -2977,7 +2979,6 @@ fuse_setxattr (xlator_t *this, fuse_in_header_t *finh, void *msg) return; } - GET_STATE (this, finh, state); state->size = fsi->size; fuse_resolve_inode_init (state, &state->resolve, finh->nodeid); @@ -3210,6 +3211,8 @@ fuse_getxattr (xlator_t *this, fuse_in_header_t *finh, void *msg) priv = this->private; + GET_STATE (this, finh, state); + #ifdef GF_DARWIN_HOST_OS if (fgxi->position) { /* position can be used only for @@ -3247,8 +3250,6 @@ fuse_getxattr (xlator_t *this, fuse_in_header_t *finh, void *msg) } } - GET_STATE (this, finh, state); - fuse_resolve_inode_init (state, &state->resolve, finh->nodeid); rv = fuse_flip_xattr_ns (priv, name, &newkey); @@ -4425,7 +4426,7 @@ fuse_thread_proc (void *data) fuse_private_t *priv = NULL; ssize_t res = 0; struct iobuf *iobuf = NULL; - fuse_in_header_t *finh; + fuse_in_header_t *finh = NULL; struct iovec iov_in[2]; void *msg = NULL; const size_t msg0_size = sizeof (*finh) + 128; diff --git a/xlators/nfs/server/src/nfs-common.c b/xlators/nfs/server/src/nfs-common.c index 781e557ba8d..07d5382a861 100644 --- a/xlators/nfs/server/src/nfs-common.c +++ b/xlators/nfs/server/src/nfs-common.c @@ -77,15 +77,15 @@ nfs_xlator_to_xlid (xlator_list_t *cl, xlator_t *xl) xlator_t * nfs_mntpath_to_xlator (xlator_list_t *cl, char *path) { - char volname[MNTPATHLEN]; - char *volptr = NULL; + char *volname = NULL; + char *volptr = NULL; size_t pathlen; - xlator_t *targetxl = NULL; + xlator_t *targetxl = NULL; if ((!cl) || (!path)) return NULL; - strcpy (volname, path); + volname = strdupa (path); pathlen = strlen (volname); gf_log (GF_NFS, GF_LOG_TRACE, "Subvolume search: %s", path); if (volname[0] == '/') @@ -101,12 +101,9 @@ nfs_mntpath_to_xlator (xlator_list_t *cl, char *path) targetxl = cl->xlator; break; } - cl = cl->next; } - return targetxl; - } diff --git a/xlators/performance/quick-read/src/quick-read.c b/xlators/performance/quick-read/src/quick-read.c index 445ea8658ea..55c5afcf877 100644 --- a/xlators/performance/quick-read/src/quick-read.c +++ b/xlators/performance/quick-read/src/quick-read.c @@ -543,8 +543,6 @@ qr_readv_cached (call_frame_t *frame, qr_inode_t *qr_inode, size_t size, LOCK (&table->lock); { - op_ret = -1; - if (!qr_inode->data) goto unlock; @@ -565,7 +563,6 @@ qr_readv_cached (call_frame_t *frame, qr_inode_t *qr_inode, size_t size, iobref = iobref_new (); if (!iobref) { op_ret = -1; - iobuf_unref (iobuf); goto unlock; } @@ -581,19 +578,15 @@ qr_readv_cached (call_frame_t *frame, qr_inode_t *qr_inode, size_t size, unlock: UNLOCK (&table->lock); - if (op_ret > 0) { + if (op_ret >= 0) { iov.iov_base = iobuf->ptr; iov.iov_len = op_ret; - STACK_UNWIND_STRICT (readv, frame, op_ret, 0, &iov, 1, &buf, iobref, xdata); } + iobuf_unref (iobuf); - if (iobuf) - iobuf_unref (iobuf); - - if (iobref) - iobref_unref (iobref); + iobref_unref (iobref); return op_ret; } diff --git a/xlators/storage/bd_map/src/bd_map.c b/xlators/storage/bd_map/src/bd_map.c index 9c8f69c6488..fcedaedb3ba 100644 --- a/xlators/storage/bd_map/src/bd_map.c +++ b/xlators/storage/bd_map/src/bd_map.c @@ -1500,11 +1500,10 @@ bd_opendir (call_frame_t *frame, xlator_t *this, op_ret = 0; out: - if (op_ret == -1) { + if (op_ret == -1 && bd_fd) { BD_PUT_ENTRY (priv, bd_fd->p_entry); - if (bd_fd) - GF_FREE (bd_fd); } + GF_FREE (bd_fd); STACK_UNWIND_STRICT (opendir, frame, op_ret, op_errno, fd, NULL); return 0; @@ -1725,7 +1724,7 @@ __bd_fill_readdir (pthread_rwlock_t *bd_lock, bd_fd_t *bd_fd, off_t off, BD_RD_LOCK (bd_lock); - bdentry = list_entry ((&bd_fd->p_entry->child)->next, typeof(*n_entry), + bdentry = list_entry ((&bd_fd->p_entry->child)->next, bd_entry_t, child); if (off) { @@ -1738,7 +1737,7 @@ __bd_fill_readdir (pthread_rwlock_t *bd_lock, bd_fd_t *bd_fd, off_t off, } } else bd_fd->entry = list_entry ((&bdentry->sibling), - typeof(*n_entry), sibling); + bd_entry_t, sibling); while (filled <= size) { cur_entry = bd_fd->entry; -- cgit