msgr: fix race in learned_addr()

- two connect() threads
- both hit if (need_addr) check
- one takes lock, sets addr, need_addr = false, unlocks
- continues to ::encode(ms_addr, ...);
- meanwhile, second thread set ms_addr _again_, but copies peer port into
  place before adjusting it.  racing ::encode() sees bad port and sends it
  to the peer.

Fix this two ways:

- don't copy bad port into place; set it first
- re-check need_addr after taking lock

Fixes: #1747
Signed-off-by: Sage Weil <sage@newdream.net>
Reviewed-by: Greg Farnum <gregory.farnum@dreamhost.com>

diff --git a/src/msg/SimpleMessenger.cc b/src/msg/SimpleMessenger.cc
index 7b02db7d00d2a92c7816292b2816075e3bf1e903..a4140544c9785173ea5e437b61adc575bd9eb14b 100644 (file)
--- a/src/msg/SimpleMessenger.cc
+++ b/src/msg/SimpleMessenger.cc
@@ -2835,13 +2835,17 @@ void SimpleMessenger::mark_disposable(Connection *con)
 
 void SimpleMessenger::learned_addr(const entity_addr_t &peer_addr_for_me)
 {
+  // be careful here: multiple threads may block here, and readers of
+  // ms_addr do NOT hold any lock.
   lock.Lock();
-  int port = ms_addr.get_port();
-  ms_addr.addr = peer_addr_for_me.addr;
-  ms_addr.set_port(port);
-  ldout(cct,1) << "learned my addr " << ms_addr << dendl;
-  need_addr = false;
-  init_local_pipe();
+  if (need_addr) {
+    entity_addr_t t = peer_addr_for_me;
+    t.set_port(ms_addr.get_port());
+    ms_addr.addr = t.addr;
+    ldout(cct,1) << "learned my addr " << ms_addr << dendl;
+    need_addr = false;
+    init_local_pipe();
+  }
   lock.Unlock();
 }