From: Loic Dachary <loic@dachary.org>
Date: Sun, 10 Feb 2013 13:23:36 +0000 (+0100)
Subject: buffer::ptr self assignment bug + patch
X-Git-Tag: v0.58~110^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=749218f155969fd87a6194b26acd00a9332d522d;p=ceph.git

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 b2d3ec6ed8c..58346df2383 100644
--- 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 4dafee8cd34..c7668a26541 100644
--- 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];