From 1a3b54ac5eb808835562990362c862e5471d0e79 Mon Sep 17 00:00:00 2001
From: Radoslaw Zarzynski <rzarzyns@redhat.com>
Date: Tue, 27 Apr 2021 14:18:32 +0000
Subject: [PATCH] crimson/monc: fix use-after-free around
 Connection::do_auth_single().

This bug caused following failure in teuthology testing [1]:

```
WARN  2021-04-23 07:08:29,449 [shard 0] osd - ms_handle_reset
WARN  2021-04-23 07:08:29,449 [shard 0] monc - active conn reset v2:172.21.15.100:3300/0
INFO  2021-04-23 07:08:29,449 [shard 0] monc - reopen_session to mon.-1
INFO  2021-04-23 07:08:29,449 [shard 0] monc - close
INFO  2021-04-23 07:08:29,449 [shard 0] monc - connecting to mon.0
...
ERROR 2021-04-23 07:08:29,450 [shard 0] none - /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/17.0.0-3454-gddedc2e1/rpm/el8/BUILD/ceph-17.0.0-3454-gddedc2e1/src/crimson/mon/MonClient.cc:228 : In function 'crimson::mon::Connection::do_auth_single(crimson::mon::Connection::request_t)::<lambda(Ref<MAuthReply>)>', ceph_assert(%s)
closed
```

[1]: http://pulpito.front.sepia.ceph.com/rzarzynski-2021-04-22_00:20:19-rados-master-distro-basic-smithi/6063316/

The root cause is in freeing the `active_con` in `Client::reopen_session()`
while there still could be the second, ongoing part of `do_auth_single()`.
This fix rectifies the issue by switching `std::unique_ptr` to `seastar::shared_ptr`
and extending the life-time with the help of `seastar::enable_shared_from_this<T>`.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
---
 src/crimson/mon/MonClient.cc | 13 +++++++------
 src/crimson/mon/MonClient.h  |  5 +++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/crimson/mon/MonClient.cc b/src/crimson/mon/MonClient.cc
index 24558ad34871e..e5a50918c58b0 100644
--- a/src/crimson/mon/MonClient.cc
+++ b/src/crimson/mon/MonClient.cc
@@ -46,7 +46,7 @@ namespace crimson::mon {
 
 using crimson::common::local_conf;
 
-class Connection {
+class Connection : public seastar::enable_shared_from_this<Connection> {
 public:
   Connection(const AuthRegistry& auth_registry,
              crimson::net::ConnectionRef conn,
@@ -224,7 +224,7 @@ Connection::do_auth_single(Connection::request_t what)
   return conn->send(m).then([this] {
     logger().info("waiting");
     return auth_reply.get_shared_future();
-  }).then([this] (Ref<MAuthReply> m) {
+  }).then([this, life_extender=shared_from_this()] (Ref<MAuthReply> m) {
     if (!m) {
       ceph_assert(closed);
       logger().info("do_auth_single: connection closed");
@@ -258,7 +258,8 @@ Connection::do_auth_single(Connection::request_t what)
 
 seastar::future<Connection::auth_result_t>
 Connection::do_auth(Connection::request_t what) {
-  return seastar::repeat_until_value([this, what]() {
+  return seastar::repeat_until_value(
+    [this, life_extender=shared_from_this(), what]() {
     return do_auth_single(what);
   });
 }
@@ -930,7 +931,7 @@ seastar::future<bool> Client::reopen_session(int rank)
   logger().info("{} to mon.{}", __func__, rank);
   if (active_con) {
     active_con->close();
-    active_con.reset();
+    active_con = nullptr;
     ceph_assert(pending_conns.empty());
   } else {
     for (auto& pending_con : pending_conns) {
@@ -958,7 +959,7 @@ seastar::future<bool> Client::reopen_session(int rank)
     logger().info("connecting to mon.{}", rank);
     auto conn = msgr.connect(peer, CEPH_ENTITY_TYPE_MON);
     auto& mc = pending_conns.emplace_back(
-      std::make_unique<Connection>(auth_registry, conn, &keyring));
+      seastar::make_shared<Connection>(auth_registry, conn, &keyring));
     assert(conn->get_peer_addr().is_msgr2());
     return mc->authenticate_v2().then([peer, this](auto result) {
       if (result == Connection::auth_result_t::success) {
@@ -1001,7 +1002,7 @@ void Client::_finish_auth(const entity_addr_t& peer)
 
   ceph_assert(!active_con && !pending_conns.empty());
   active_con = std::move(*found);
-  found->reset();
+  *found = nullptr;
   for (auto& conn : pending_conns) {
     if (conn) {
       conn->close();
diff --git a/src/crimson/mon/MonClient.h b/src/crimson/mon/MonClient.h
index 9ae4d2a79e8f5..cd15b2cd22d42 100644
--- a/src/crimson/mon/MonClient.h
+++ b/src/crimson/mon/MonClient.h
@@ -9,6 +9,7 @@
 #include <seastar/core/future.hh>
 #include <seastar/core/gate.hh>
 #include <seastar/core/lowres_clock.hh>
+#include <seastar/core/shared_ptr.hh>
 #include <seastar/core/timer.hh>
 
 #include "auth/AuthRegistry.h"
@@ -54,8 +55,8 @@ class Client : public crimson::net::Dispatcher,
   const uint32_t want_keys;
 
   MonMap monmap;
-  std::unique_ptr<Connection> active_con;
-  std::vector<std::unique_ptr<Connection>> pending_conns;
+  seastar::shared_ptr<Connection> active_con;
+  std::vector<seastar::shared_ptr<Connection>> pending_conns;
   seastar::timer<seastar::lowres_clock> timer;
 
   crimson::net::Messenger& msgr;
-- 
2.39.5