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 <sage@inktank.com>
(cherry picked from commit 3ec32a6bb11d92e36a0e6381b40ce2fd1fbb016a)

diff --git a/src/librados/librados.cc b/src/librados/librados.cc
index 2358fb406e8c5abe0c949dc89811cc9f857bdce5..26b94bd591b8e4318e396b5b45af5610fca8032e 100644 (file)
--- 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<std::string>::const_iterator i = pools.begin();
   std::list<std::string>::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 65d5c2263315aba1a671e1ee8b548db0873f407d..04286fcf32a54a868473b92414f95f1deab5bd8f 100644 (file)
--- 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<char> 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));
 }