fix buffer::list::zero(unsigned o, unsigned l) to act on all buffer::ptr

When buffer::list::zero was called on a buffer::list with "ABC" and
"DEF" in two different buffer::ptr with buffer::list::zero(4, 1) it
did nothing. The expected result is that the "DEF" buffer::ptr is
modified to "D\0F"

The test to check if the pointer is past the end of range to be zeroed
was reversed. It was o+l >= p which would always be true if the range
spans over the first buffer::ptr . It must be "o+l <= p" meaning the
pointer is past the end of the range and there is no need to loop over
the remaining buffer::ptr in the buffer::list

The p+it->length() >= o+l part of the if (p >= o && p+it->length() >=
o+l) test was also reversed. When called on "ABC" with zero(0, 1) it
would match because the range to be zeroed is contained in the
buffer::ptr. The call to it->zero() would zero the entire buffer::ptr
instead of just the first character.

unit tests are added to demonstrate the two problems with the previous
code and show that the patch fixes them.

http://tracker.ceph.com/issues/4123 refs #4123

Signed-off-by: Loic Dachary <loic@dachary.org>

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index 58346df2383a3b49d3eb41c21a40d343434045e8..e10d6c9038c2ffc9992c3f8fb2652603456421f4 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -736,7 +736,7 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
         it != _buffers.end();
         it++) {
       if (p + it->length() > o) {
-       if (p >= o && p+it->length() >= o+l)
+       if (p >= o && p+it->length() <= o+l)
          it->zero();                         // all
        else if (p >= o) 
          it->zero(0, o+l-p);                 // head
@@ -744,7 +744,7 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
          it->zero(o-p, it->length()-(o-p));  // tail
       }
       p += it->length();
-      if (o+l >= p)
+      if (o+l <= p)
        break;  // done
     }
   }

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index c7668a26541c24c693f1215979b31b874758703d..91e37e6b9f5b3d054101fdae6812e68060a34b6f 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -9,6 +9,53 @@
 
 #define MAX_TEST 1000000
 
+TEST(BufferList, zero) {
+  //
+  // void zero()
+  //
+  {
+    bufferlist bl;
+    bl.append('A');
+    EXPECT_EQ('A', bl[0]);
+    bl.zero();
+    EXPECT_EQ('\0', bl[0]);
+  }
+  //
+  // void zero(unsigned o, unsigned l)
+  //
+  const char *s[] = {
+    "ABC",
+    "DEF",
+    "GHI",
+    "KLM"
+  };
+  {
+    bufferlist bl;
+    bufferptr ptr(s[0], strlen(s[0]));
+    bl.push_back(ptr);
+    bl.zero((unsigned)0, (unsigned)1);
+    EXPECT_EQ(0, ::memcmp("\0BC", bl.c_str(), 3));
+  }
+  {
+    bufferlist bl;
+    for (unsigned i = 0; i < 4; i++) {
+      bufferptr ptr(s[i], strlen(s[i]));
+      bl.push_back(ptr);
+    }
+    EXPECT_THROW(bl.zero((unsigned)0, (unsigned)2000), FailedAssertion);
+    bl.zero((unsigned)2, (unsigned)5);
+    EXPECT_EQ(0, ::memcmp("AB\0\0\0\0\0HIKLM", bl.c_str(), 9));
+  }
+  {
+    bufferlist bl;
+    for (unsigned i = 0; i < 4; i++) {
+      bufferptr ptr(s[i], strlen(s[i]));
+      bl.push_back(ptr);
+    }
+    bl.zero((unsigned)3, (unsigned)3);
+    EXPECT_EQ(0, ::memcmp("ABC\0\0\0GHIKLM", bl.c_str(), 9));
+  }
+}
 
 TEST(BufferList, EmptyAppend) {
   bufferlist bl;