From: Sage Weil <sage@inktank.com>
Date: Tue, 3 Jun 2014 18:45:20 +0000 (-0700)
Subject: librados: simplify/fix rados_pool_list bounds checks
X-Git-Tag: v0.82~4^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F1910%2Fhead;p=ceph.git

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>
---

diff --git a/src/librados/librados.cc b/src/librados/librados.cc
index 2358fb406e8c..26b94bd591b8 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<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 65d5c2263315..04286fcf32a5 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<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));
 }