From 500206d809f0cd85cd99e4f0ec164bbf74f92c28 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 00:04:05 +0100 Subject: [PATCH 01/12] ReplicatedPG.cc: fix ressource leak, delete cb CID 1188145 (#1 of 1): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "cb" going out of scope leaks the storage it points to. Signed-off-by: Danny Al-Gaaf --- src/osd/ReplicatedPG.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc index 32171c3ba4bbf..52a7690ab2f9c 100644 --- a/src/osd/ReplicatedPG.cc +++ b/src/osd/ReplicatedPG.cc @@ -5227,8 +5227,12 @@ int ReplicatedPG::fill_in_copy_get( ctx->obc, &out_attrs, true); - if (result < 0) + if (result < 0) { + if (cb) { + delete cb; + } return result; + } cursor.attr_complete = true; dout(20) << " got attrs" << dendl; } @@ -5250,6 +5254,9 @@ int ReplicatedPG::fill_in_copy_get( result = pgbackend->objects_read_sync( oi.soid, cursor.data_offset, left, &bl); if (result < 0) + if (cb) { + delete cb; + } return result; } assert(result <= left); From 7eefe85cf53ef58349fb44af2ac60d7ebb9f5960 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 00:19:58 +0100 Subject: [PATCH 02/12] histogram.h: fix potential div by zero CID 1188131 (#1 of 1): Division or modulo by zero (DIVIDE_BY_ZERO) divide_by_zero: In expression "lower_sum * 1000000UL / total", division by expression "total" which may be zero has undefined behavior Added check for non zero total. Signed-off-by: Danny Al-Gaaf --- src/common/histogram.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/common/histogram.h b/src/common/histogram.h index 8d1ad8ea29a3b..bdbd75ca132a2 100644 --- a/src/common/histogram.h +++ b/src/common/histogram.h @@ -94,8 +94,10 @@ struct pow2_hist_t { // lower_sum += h[i]; total += h[i]; } - *lower = lower_sum * 1000000 / total; - *upper = upper_sum * 1000000 / total; + if (total > 0) { + *lower = lower_sum * 1000000 / total; + *upper = upper_sum * 1000000 / total; + } return 0; } From 0bf5f8668fdc1b0b6167dabb02e17c4cdf7e83cc Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 01:24:37 +0100 Subject: [PATCH 03/12] store_test.cc: fix unchecked return value CID 1188126 (#1 of 1): Unchecked return value (CHECKED_RETURN) 2. check_return: Calling function "ObjectStore::stat(coll_t, ghobject_t const &, stat *, bool)" without checking return value (as is done elsewhere 8 out of 9 times). 3. unchecked_value: No check of the return value of "this->store->stat( coll_t(this->cid), hoid, &buf, false)". Signed-off-by: Danny Al-Gaaf --- src/test/objectstore/store_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index b9c74abaf2eed..8133d4dc0e777 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -561,7 +561,8 @@ class SyntheticWorkloadState { ++in_flight; } struct stat buf; - store->stat(cid, hoid, &buf); + int r = store->stat(cid, hoid, &buf); + ASSERT_EQ(0, r); ASSERT_TRUE(buf.st_size == contents[hoid].length()); { Mutex::Locker locker(lock); From e8533ee4c9fb5dbe06373f2f0a194a81ea3cf25a Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 11:16:27 +0100 Subject: [PATCH 04/12] ReplicatedBackend: check result of dynamic_cast to fix null pointer deref CID 1188135 (#1 of 1): Unchecked dynamic_cast (FORWARD_NULL) var_deref_model: Passing null pointer "t" to function "RPGTransaction::get_transaction()", which dereferences it CID 1188134 (#1 of 1): Unchecked dynamic_cast (FORWARD_NULL) var_deref_op: Dereferencing null pointer "to_append". Signed-off-by: Danny Al-Gaaf --- src/osd/ReplicatedBackend.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/osd/ReplicatedBackend.cc b/src/osd/ReplicatedBackend.cc index 6b9e477f563cb..4eb6005c7c224 100644 --- a/src/osd/ReplicatedBackend.cc +++ b/src/osd/ReplicatedBackend.cc @@ -425,6 +425,7 @@ class RPGTransaction : public PGBackend::PGTransaction { PGTransaction *_to_append ) { RPGTransaction *to_append = dynamic_cast(_to_append); + assert(to_append); t->append(*(to_append->t)); for (set::iterator i = to_append->temp_added.begin(); i != to_append->temp_added.end(); @@ -492,6 +493,7 @@ void ReplicatedBackend::submit_transaction( OpRequestRef orig_op) { RPGTransaction *t = dynamic_cast(_t); + assert(t); ObjectStore::Transaction *op_t = t->get_transaction(); assert(t->get_temp_added().size() <= 1); From ad9b6d2f7a0220354b28645ae15fbb7ed734c87c Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 11:36:18 +0100 Subject: [PATCH 05/12] c_write_operations.cc: fix some ioctx resource leaks CID 1160833 (#3 of 3): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "ioctx" going out of scope leaks the storage it points to CID 1160835 (#3 of 3): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "ioctx" going out of scope leaks the storage it points to. CID 1188156 (#5 of 5): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "ioctx" going out of scope leaks the storage it points to. Signed-off-by: Danny Al-Gaaf --- src/test/librados/c_write_operations.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/librados/c_write_operations.cc b/src/test/librados/c_write_operations.cc index d511172e2f034..4dede36a36792 100644 --- a/src/test/librados/c_write_operations.cc +++ b/src/test/librados/c_write_operations.cc @@ -73,6 +73,7 @@ TEST(LibRadosCWriteOps, Xattrs) { ASSERT_EQ(-125, rados_write_op_operate(op, ioctx, "test", NULL, 0)); rados_release_write_op(op); + rados_ioctx_destroy(ioctx); ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster)); } @@ -113,6 +114,7 @@ TEST(LibRadosCWriteOps, Write) { ASSERT_EQ(-2, rados_read(ioctx, "test", hi, 4, 0)); rados_release_write_op(op); + rados_ioctx_destroy(ioctx); ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster)); } @@ -135,5 +137,6 @@ TEST(LibRadosCWriteOps, Exec) { hi[12] = '\0'; ASSERT_EQ(0, strcmp("Hello, test!", hi)); + rados_ioctx_destroy(ioctx); ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster)); } From 3cd751b0a280909510c3e633cc8cd4b9f5e3b2d9 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 11:44:39 +0100 Subject: [PATCH 06/12] c_read_operations.cc: fix resource leak CID 1188154 (#2 of 2): Resource leak (RESOURCE_LEAK) overwrite_var: Overwriting "op" in "op = rados_create_read_op()" leaks the storage that "op" points to. Signed-off-by: Danny Al-Gaaf --- src/test/librados/c_read_operations.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/librados/c_read_operations.cc b/src/test/librados/c_read_operations.cc index 106f6b4f162d9..053b6147ba511 100644 --- a/src/test/librados/c_read_operations.cc +++ b/src/test/librados/c_read_operations.cc @@ -365,6 +365,7 @@ TEST_F(CReadOpsTest, ExecUserBuf) { // buffer too short bytes_read = 1024; + rados_release_read_op(op); op = rados_create_read_op(); rados_read_op_exec_user_buf(op, "rbd", "get_all_features", NULL, 0, out, sizeof(features) - 1, &bytes_read, &rval); From 249e21079257fafe06d80c541f52751e8bf942b2 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 11:53:09 +0100 Subject: [PATCH 07/12] FileStore: fix resource leak in queue_transactions() blackhole case CID 1135931 (#1 of 1): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "ondisk" going out of scope leaks the storage it points to. CID 1135932 (#1 of 1): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "onreadable" going out of scope leaks the storage it points to. CID 1135933 (#1 of 1): Resource leak (RESOURCE_LEAK) leaked_storage: Variable "onreadable_sync" going out of scope leaks the storage it points to. Signed-off-by: Danny Al-Gaaf --- src/os/FileStore.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/os/FileStore.cc b/src/os/FileStore.cc index 376c39776db7d..cef1d187c61dc 100644 --- a/src/os/FileStore.cc +++ b/src/os/FileStore.cc @@ -1736,6 +1736,9 @@ int FileStore::queue_transactions(Sequencer *posr, list &tls, tls, &onreadable, &ondisk, &onreadable_sync); if (g_conf->filestore_blackhole) { dout(0) << "queue_transactions filestore_blackhole = TRUE, dropping transaction" << dendl; + delete ondisk; + delete onreadable; + delete onreadable_sync; return 0; } From 93c09836fe7aeebc8cee0dff47ccd94f3a2c42e8 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 12:10:56 +0100 Subject: [PATCH 08/12] MDCache::handle_discover: fix null pointer deref CID 716990 (#1 of 1): Dereference null return value (NULL_RETURNS) dereference: Dereferencing a pointer that might be null "cur" when calling "MDCache::replicate_inode(CInode *, int, ceph::bufferlist &)" Add assert to check for return value from get_inode() as done in other places. Signed-off-by: Danny Al-Gaaf --- src/mds/MDCache.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc index f90df94efb2f8..14416c62986c9 100644 --- a/src/mds/MDCache.cc +++ b/src/mds/MDCache.cc @@ -10112,6 +10112,7 @@ void MDCache::handle_discover(MDiscover *dis) << dendl; cur = get_inode(dis->get_base_ino()); + assert(cur); // add root reply->starts_with = MDiscoverReply::INODE; From 1747c589e7731dea1068d1d020dc2774eb92ae46 Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 13:11:48 +0100 Subject: [PATCH 09/12] MDCache: fix potential null pointer deref CID 716921 (#1 of 1): Dereference after null check (FORWARD_NULL) var_deref_model: Passing null pointer "dir" to function "operator <<(std::ostream &, CDir &)", which dereferences it. Signed-off-by: Danny Al-Gaaf --- src/mds/MDCache.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc index 14416c62986c9..080fe1bd05e32 100644 --- a/src/mds/MDCache.cc +++ b/src/mds/MDCache.cc @@ -6973,8 +6973,12 @@ void MDCache::handle_cache_expire(MCacheExpire *m) dn = dir->lookup(p->first.first, p->first.second); } - if (!dn) - dout(0) << " missing dentry for " << p->first.first << " snap " << p->first.second << " in " << *dir << dendl; + if (!dn) { + if (dir) + dout(0) << " missing dentry for " << p->first.first << " snap " << p->first.second << " in " << *dir << dendl; + else + dout(0) << " missing dentry for " << p->first.first << " snap " << p->first.second << dendl; + } assert(dn); if (nonce == dn->get_replica_nonce(from)) { From 754a36897b3b1454e51df40c623e216f159874cd Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 13:33:18 +0100 Subject: [PATCH 10/12] PGMonitor: fix uninitialized scalar variable Fix type handling in dump_stuck_pg_stats. If type is type doesn't match to known PGMap::STUCK_* type print out a message and return directly from function. CID 1030132 (#2 of 2): Uninitialized scalar variable (UNINIT) uninit_use_in_call: Using uninitialized value "stuck_type" when calling "PGMap::dump_stuck(ceph::Formatter *, PGMap::StuckPG, utime_t) const" Signed-off-by: Danny Al-Gaaf --- src/mon/PGMonitor.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/mon/PGMonitor.cc b/src/mon/PGMonitor.cc index 6dea1c9369b04..1c64cdc8ec5ae 100644 --- a/src/mon/PGMonitor.cc +++ b/src/mon/PGMonitor.cc @@ -1978,12 +1978,17 @@ int PGMonitor::dump_stuck_pg_stats(stringstream &ds, { PGMap::StuckPG stuck_type; string type = args[0]; + if (type == "inactive") stuck_type = PGMap::STUCK_INACTIVE; - if (type == "unclean") + else if (type == "unclean") stuck_type = PGMap::STUCK_UNCLEAN; - if (type == "stale") + else if (type == "stale") stuck_type = PGMap::STUCK_STALE; + else { + ds << "Unknown type: " << type << std::endl; + return 0; + } utime_t now(ceph_clock_now(g_ceph_context)); utime_t cutoff = now - utime_t(threshold, 0); From 605e645026487519d4195358330832b3369b531d Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 13:45:53 +0100 Subject: [PATCH 11/12] Objecter::recalc_op_target: fix uninitialized scalar variable CID 1160848 (#1 of 1): Uninitialized scalar variable (UNINIT) uninit_use: Using uninitialized value "best". Init 'best' with -1 (from the code logic it will be set at least to 0) to silence coverity. Signed-off-by: Danny Al-Gaaf --- src/osdc/Objecter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc index 7f64b00eca1da..a2954662605cf 100644 --- a/src/osdc/Objecter.cc +++ b/src/osdc/Objecter.cc @@ -1495,7 +1495,7 @@ int Objecter::recalc_op_target(Op *op) acting.size() > 1) { // look for a local replica. prefer the primary if the // distance is the same. - int best; + int best = -1; int best_locality; for (unsigned i = 0; i < acting.size(); ++i) { int locality = osdmap->crush->get_common_ancestor_distance( From 1a4657a374552b62a4d950c55f4ed5d4d029503a Mon Sep 17 00:00:00 2001 From: Danny Al-Gaaf Date: Sat, 1 Mar 2014 14:26:18 +0100 Subject: [PATCH 12/12] req_state: fix uninitialized bool var CID 717359 (#1 of 1): Uninitialized scalar field (UNINIT_CTOR) uninit_member: Non-static class member "bucket_exists" is not initialized in this constructor nor in any functions that it calls. Set bucket_exists to false in req_state::req_state(). Signed-off-by: Danny Al-Gaaf --- src/rgw/rgw_common.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rgw/rgw_common.cc b/src/rgw/rgw_common.cc index 57b8cebb4909b..50b0eaf32bf37 100644 --- a/src/rgw/rgw_common.cc +++ b/src/rgw/rgw_common.cc @@ -151,6 +151,7 @@ req_state::req_state(CephContext *_cct, class RGWEnv *e) : cct(_cct), cio(NULL), perm_mask = 0; content_length = 0; object = NULL; + bucket_exists = false; has_bad_meta = false; length = NULL; copy_source = NULL;