From 6e88d2768501d99d60986f7fe1e74905e22e1087 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 17 Nov 2009 16:30:36 -0800 Subject: [PATCH] msgr: fix possible use-after-free by taking pipe lock during reap This ensures that whoever called stop() (mark_down, in particular) finished with the pipe (unlocked it) before we go and free it. Otherwise we might call p->lock.Unlock() after the reaper deleted the Pipe, mucking up some other memory. I don't think this was actually triggerd, tho, since we would have seen the assert(nlock == 0) in ~Mutex??? --- src/msg/SimpleMessenger.cc | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/msg/SimpleMessenger.cc b/src/msg/SimpleMessenger.cc index a0bbae6c45fd8..88599a3172380 100644 --- a/src/msg/SimpleMessenger.cc +++ b/src/msg/SimpleMessenger.cc @@ -1876,9 +1876,11 @@ void SimpleMessenger::reaper() while (!pipe_reap_queue.empty()) { Pipe *p = pipe_reap_queue.front(); + pipe_reap_queue.pop_front(); dout(10) << "reaper reaping pipe " << p << " " << p->get_peer_addr() << dendl; + p->lock.Lock(); + p->lock.Unlock(); p->unregister_pipe(); - pipe_reap_queue.pop_front(); assert(pipes.count(p)); pipes.erase(p); p->join(); @@ -2241,18 +2243,13 @@ void SimpleMessenger::wait() lock.Lock(); { dout(10) << "wait: closing pipes" << dendl; - list toclose; - for (hash_map::iterator i = rank_pipe.begin(); - i != rank_pipe.end(); - i++) - toclose.push_back(i->second); - for (list::iterator i = toclose.begin(); - i != toclose.end(); - i++) { - (*i)->unregister_pipe(); - (*i)->lock.Lock(); - (*i)->stop(); - (*i)->lock.Unlock(); + + while (!rank_pipe.empty()) { + Pipe *p = rank_pipe.begin()->second; + p->unregister_pipe(); + p->lock.Lock(); + p->stop(); + p->lock.Unlock(); } reaper(); -- 2.39.5