msgr: ref count Pipe to avoid use after free

The Connection has a Pipe pointer to facilitate
 send_message(Message, Connection)
but the reaper() clears that pointer when tearing down old pipes.  This
leads to a race in which submit_message dereferences the old Pipe pointer.

Instead, make Pipe ref counted, and only submit_message() if we get a
valid Pipe reference.  This fixes races between send_message() and
reaper() (as well as any use of the Connection after the pipe is closed).

Signed-off-by: Sage Weil <sage@newdream.net>

diff --git a/src/msg/Message.h b/src/msg/Message.h
index 5621a22c5eeb95eed1e6afbab315447e90fdd4ff..a5eda52c980249979a4168cc01203cd340026e98 100644 (file)
--- a/src/msg/Message.h
+++ b/src/msg/Message.h
@@ -146,12 +146,12 @@ struct RefCountedObject {
   virtual ~RefCountedObject() {}
   
   RefCountedObject *get() {
-    //generic_dout(0) << "RefCountedObject::get " << this << " " << nref.test() << " -> " << (nref.test() + 1) << dendl;
+    //generic_dout(0) << "RefCountedObject::get " << this << " " << nref.read() << " -> " << (nref.read() + 1) << dendl;
     nref.inc();
     return this;
   }
   void put() {
-    //generic_dout(0) << "RefCountedObject::put " << this << " " << nref.test() << " -> " << (nref.test() - 1) << dendl;
+    //generic_dout(0) << "RefCountedObject::put " << this << " " << nref.read() << " -> " << (nref.read() - 1) << dendl;
     if (nref.dec() == 0)
       delete this;
   }
@@ -164,7 +164,7 @@ struct Connection : public RefCountedObject {
   int peer_type;
   entity_addr_t peer_addr;
   unsigned features;
-  void *pipe;
+  RefCountedObject *pipe;
 
 public:
   Connection() : nref(1), lock("Connection::lock"), priv(NULL), peer_type(-1), features(0), pipe(NULL) {}
@@ -174,6 +174,8 @@ public:
       //generic_dout(0) << "~Connection " << this << " dropping priv " << priv << dendl;
       priv->put();
     }
+    if (pipe)
+      pipe->put();
   }
 
   Connection *get() {
@@ -198,6 +200,20 @@ public:
     return NULL;
   }
 
+  RefCountedObject *get_pipe() {
+    Mutex::Locker l(lock);
+    if (pipe)
+      return pipe->get();
+    return NULL;
+  }
+  void clear_pipe() {
+    Mutex::Locker l(lock);
+    if (pipe) {
+      pipe->put();
+      pipe = NULL;
+    }
+  }
+
   int get_peer_type() { return peer_type; }
   void set_peer_type(int t) { peer_type = t; }
   

diff --git a/src/msg/SimpleMessenger.cc b/src/msg/SimpleMessenger.cc
index a30e0759ae4be138f57bdd6388ffba064815991e..dcc27eb5d5a5b7de305ef0e5b4be5c82792b81f4 100644 (file)
--- a/src/msg/SimpleMessenger.cc
+++ b/src/msg/SimpleMessenger.cc
@@ -421,7 +421,11 @@ int SimpleMessenger::send_message(Message *m, Connection *con)
          << " " << m
          << dendl;
 
-  submit_message(m, (SimpleMessenger::Pipe *)con->pipe);
+  SimpleMessenger::Pipe *pipe = (SimpleMessenger::Pipe *)con->get_pipe();
+  if (pipe) {
+    submit_message(m, pipe);
+    pipe->put();
+  } // else we raced with reaper()
   return 0;
 }
 
@@ -2113,7 +2117,9 @@ void SimpleMessenger::reaper()
     p->join();
     dout(10) << "reaper reaped pipe " << p << " " << p->get_peer_addr() << dendl;
     assert(p->sd < 0);
-    delete p;
+    if (p->connection_state)
+      p->connection_state->clear_pipe();
+    p->put();
     dout(10) << "reaper deleted pipe " << p << dendl;
   }
 }

diff --git a/src/msg/SimpleMessenger.h b/src/msg/SimpleMessenger.h
index a3a58f4a72799a5b0a604cee34375572323879a7..968babf8ab86e9f3fc654e2c95d6deb6f6230ba0 100644 (file)
--- a/src/msg/SimpleMessenger.h
+++ b/src/msg/SimpleMessenger.h
@@ -95,7 +95,7 @@ private:
   void sigint(int r);
 
   // pipe
-  class Pipe {
+  class Pipe : public RefCountedObject {
   public:
     SimpleMessenger *messenger;
     ostream& _pipe_prefix();
@@ -189,7 +189,7 @@ private:
       connect_seq(0), peer_global_seq(0),
       out_seq(0), in_seq(0), in_seq_acked(0),
       reader_thread(this), writer_thread(this) {
-      connection_state->pipe = this;
+      connection_state->pipe = get();
     }
     ~Pipe() {
       for (map<int, xlist<Pipe *>::item* >::iterator i = queue_items.begin();
@@ -201,7 +201,6 @@ private:
       }
       assert(out_q.empty());
       assert(sent.empty());
-      connection_state->pipe = NULL;
       connection_state->put();
     }