From 6aa7f7ee4e082819decc48da1f0eaffc4e616302 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 3 Jun 2014 11:45:20 -0700 Subject: [PATCH] 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 (cherry picked from commit 3ec32a6bb11d92e36a0e6381b40ce2fd1fbb016a) --- 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 2358fb406e8c5..26b94bd591b8e 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 65d5c2263315a..04286fcf32a54 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)); } -- 2.39.5