From 3ec32a6bb11d92e36a0e6381b40ce2fd1fbb016a Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 3 Jun 2014 11:45:20 -0700 Subject: [PATCH 1/2] librados: simplify/fix rados_pool_list bounds checks We were not breaking out of the loop when we filled up the buffer unless we happened to do so on a pool name boundary. This means that len would roll over (it was unsigned). In my case, I was not able to reproduce anything particularly bad since (I think) the strncpy was interpreting the large unsigned value as signed, but in any case this fixes it, simplifies the arithmetic, and adds a simple test. - use a single 'rl' value for the amount of buffer space we want to consume - use this to check that there is room and also as the strncat length - rely on the initial memset to ensure that the trailing 0 is in place. Fixes: #8447 Signed-off-by: Sage Weil --- src/librados/librados.cc | 6 +++--- src/test/librados/pool.cc | 12 ++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/librados/librados.cc b/src/librados/librados.cc index 2358fb406e8..26b94bd591b 100644 --- a/src/librados/librados.cc +++ b/src/librados/librados.cc @@ -2058,10 +2058,10 @@ extern "C" int rados_pool_list(rados_t cluster, char *buf, size_t len) std::list::const_iterator i = pools.begin(); std::list::const_iterator p_end = pools.end(); for (; i != p_end; ++i) { - if (len == 0) - break; int rl = i->length() + 1; - strncat(b, i->c_str(), len - 2); // leave space for two NULLs + if (len < (unsigned)rl) + break; + strncat(b, i->c_str(), rl); needed += rl; len -= rl; b += rl; diff --git a/src/test/librados/pool.cc b/src/test/librados/pool.cc index 65d5c226331..04286fcf32a 100644 --- a/src/test/librados/pool.cc +++ b/src/test/librados/pool.cc @@ -8,8 +8,8 @@ #define POOL_LIST_BUF_SZ 32768 TEST(LibRadosPools, PoolList) { - std::vector pool_list_buf(POOL_LIST_BUF_SZ, '\0'); - char *buf = &pool_list_buf[0]; + char pool_list_buf[POOL_LIST_BUF_SZ]; + char *buf = pool_list_buf; rados_t cluster; std::string pool_name = get_temp_pool_name(); ASSERT_EQ("", create_one_pool(pool_name, &cluster)); @@ -23,6 +23,14 @@ TEST(LibRadosPools, PoolList) { buf += strlen(buf) + 1; } ASSERT_EQ(found_pool, true); + + // make sure we honor the buffer size limit + buf = pool_list_buf; + memset(buf, 0, POOL_LIST_BUF_SZ); + ASSERT_LT(rados_pool_list(cluster, buf, 20), POOL_LIST_BUF_SZ); + ASSERT_NE(0, buf[0]); // include at least one pool name + ASSERT_EQ(0, buf[20]); // but don't touch the stopping point + ASSERT_EQ(0, destroy_one_pool(pool_name, &cluster)); } From 2081c992bbe3a83d711f465634d19c011d28ea3e Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Thu, 5 Jun 2014 10:43:16 -0700 Subject: [PATCH 2/2] include/atomic: make 32-bit atomic64_t unsigned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes In file included from test/perf_counters.cc:19:0: ./common/perf_counters.h: In member function ‘std::pair PerfCounters::perf_counter_data_any_d::read_avg() const’: warning: ./common/perf_counters.h:156:36: comparison between signed and unsigned integer expressions [-Wsign-compare] } while (avgcount2.read() != count); ^ Signed-off-by: Sage Weil --- src/include/atomic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/atomic.h b/src/include/atomic.h index 55f7290ac87..88c9bfd3871 100644 --- a/src/include/atomic.h +++ b/src/include/atomic.h @@ -122,7 +122,7 @@ namespace ceph { #if SIZEOF_AO_T == 8 typedef atomic_t atomic64_t; #else - typedef atomic_spinlock_t atomic64_t; + typedef atomic_spinlock_t atomic64_t; #endif }