buffer::ptr self assignment bug + patch

After

buffer::ptr a(1);
a = a;

a call to a.get_raw() will return a null pointer and there will be
no pointer referencing the original buffer::raw object although its
reference count is 1.

buffer::ptr& buffer::ptr::operator= (const ptr& p) is modified to use
a local buffer::raw pointer to fix the memory leak. a = a
is a noop instead of loosing the original raw buffer.

A set of unit tests is added src/test/bufferlist.cc to demonstrate
that the proposed change works as expected. It is checked with
valgrind that reports no memory leak. The same test can be run against
the original code to show that it leaks.

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

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

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index b2d3ec6ed8c9b706a919c312d0e8fa265b0bf14d..58346df2383a3b49d3eb41c21a40d343434045e8 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -285,14 +285,14 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
   }
   buffer::ptr& buffer::ptr::operator= (const ptr& p)
   {
-    // be careful -- we need to properly handle self-assignment.
     if (p._raw) {
-      p._raw->nref.inc();                      // inc new
+      p._raw->nref.inc();
       bdout << "ptr " << this << " get " << _raw << bendl;
     }
-    release();                                 // dec (+ dealloc) old (if any)
-    if (p._raw) {
-      _raw = p._raw;
+    buffer::raw *raw = p._raw; 
+    release();
+    if (raw) {
+      _raw = raw;
       _off = p._off;
       _len = p._len;
     } else {

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 4dafee8cd3454ee053b1265def0e2c3e2b4ea546..c7668a26541c24c693f1215979b31b874758703d 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -40,6 +40,54 @@ TEST(BufferList, TestPtrAppend) {
   ASSERT_EQ(memcmp(bl.c_str(), correct, curpos), 0);
 }
 
+TEST(BufferList, ptr_assignment) {
+  unsigned len = 17;
+  //
+  // override a bufferptr set with the same raw
+  //
+  {
+    bufferptr original(len);
+    bufferptr same_raw(original.get_raw());
+    unsigned offset = 5;
+    unsigned length = len - offset;
+    original.set_offset(offset);
+    original.set_length(length);
+    same_raw = original;
+    ASSERT_EQ(2, original.raw_nref());
+    ASSERT_EQ(same_raw.get_raw(), original.get_raw());
+    ASSERT_EQ(same_raw.offset(), original.offset());
+    ASSERT_EQ(same_raw.length(), original.length());
+  }
+
+  //
+  // self assignment is a noop
+  //
+  {
+    bufferptr original(len);
+    original = original;
+    ASSERT_EQ(1, original.raw_nref());
+    ASSERT_EQ((unsigned)0, original.offset());
+    ASSERT_EQ(len, original.length());
+  }
+  
+  //
+  // a copy points to the same raw
+  //
+  {
+    bufferptr original(len);
+    unsigned offset = 5;
+    unsigned length = len - offset;
+    original.set_offset(offset);
+    original.set_length(length);
+    bufferptr ptr;
+    ptr = original;
+    ASSERT_EQ(2, original.raw_nref());
+    ASSERT_EQ(ptr.get_raw(), original.get_raw());
+    ASSERT_EQ(original.offset(), ptr.offset());
+    ASSERT_EQ(original.length(), ptr.length());
+  }
+}
+
 TEST(BufferList, TestDirectAppend) {
   bufferlist bl;
   char correct[MAX_TEST];