Merge pull request oxen-io#134 from jagerman/btstreamreq-crash-fix

Fix crash when stream buffer outlives the sent_req

include/oxen/quic/btstream.hpp

@@ -154,9 +154,6 @@ namespace oxen::quic
 
         message to_timeout() && { return {return_sender, ""_bs, true}; }
 
-        std::string_view view() { return {data}; }
-        std::string payload() && { return std::move(data); }
-
       private:
         void handle_req_opts(std::function<void(message)> func) { cb = std::move(func); }
         void handle_req_opts(std::chrono::milliseconds exp) { timeout = exp; }
@@ -227,10 +224,10 @@ namespace oxen::quic
             if (req->cb)
                 endpoint.call([this, r = std::move(req)]() mutable {
                     if (auto* req = add_sent_request(std::move(r)))
-                        send(req->view());
+                        send(std::move(req->data));
                 });
             else
-                send(std::move(*req).payload());
+                send(std::move(*req).data);
         }
         // Same as above, but takes a regular string_view
         template <typename... Opt>

include/oxen/quic/endpoint.hpp

@@ -182,6 +182,9 @@ namespace oxen::quic
             net.call_soon(std::forward<Args>(args)...);
         }
 
+        // Defers destruction of a shared_ptr to a future (but not current) event loop tick.
+        void reset_soon(std::shared_ptr<void> ptr) { net.reset_soon(std::move(ptr)); }
+
         // Shortcut for calling net.make_shared<T> to make a std::shared_ptr<T> that has destruction
         // synchronized to the network event loop.
         template <typename T, typename... Args>

include/oxen/quic/network.hpp

@@ -57,6 +57,11 @@ namespace oxen::quic
             return _loop->call_get(std::forward<Callable>(f));
         }
 
+        void reset_soon(std::shared_ptr<void> ptr)
+        {
+            call_soon([ptr = std::move(ptr)]() mutable { ptr.reset(); });
+        }
+
       private:
         std::shared_ptr<Loop> _loop;
         std::atomic<bool> shutdown_immediate{false};

src/btstream.cpp

@@ -59,7 +59,7 @@ namespace oxen::quic
     {
         log::trace(bp_cat, "{} called", __PRETTY_FUNCTION__);
 
-        send(sent_request{*this, encode_response(rid, body, error), rid}.payload());
+        send(sent_request{*this, encode_response(rid, body, error), rid}.data);
     }
 
     void BTRequestStream::check_timeouts()

src/connection.cpp

@@ -803,7 +803,10 @@ namespace oxen::quic
             assert(n_packets > 0);  // n_packets, buf, bufsize now contain the unsent packets
             log::debug(log_cat, "Packet send blocked; queuing re-send");
 
-            _endpoint.get_socket()->when_writeable([this] {
+            _endpoint.get_socket()->when_writeable([&ep = _endpoint, connid = reference_id(), this] {
+                if (!ep.conns.count(connid))
+                    return;  // Connection has gone away (and so `this` isn't valid!)
+
                 if (send(nullptr))
                 {  // Send finished so we can start our timers up again
                     packet_io_ready();
@@ -819,10 +822,8 @@ namespace oxen::quic
             if (pkt_updater)
                 pkt_updater->cancel();
 
-            _endpoint.call([this]() {
-                log::debug(log_cat, "Endpoint deleting {}", reference_id());
-                _endpoint.drop_connection(*this, io_error{CONN_SEND_FAIL});
-            });
+            log::debug(log_cat, "Endpoint deleting {}", reference_id());
+            _endpoint.drop_connection(*this, io_error{CONN_SEND_FAIL});
 
             return false;
         }
@@ -1608,7 +1609,8 @@ namespace oxen::quic
             remote_pubkey = *remote_pk;
             tls_session->set_expected_remote_key(remote_pubkey);
 
-            if (auto maybe_token = _endpoint.get_path_validation_token(remote_pubkey))
+            auto maybe_token = _endpoint.get_path_validation_token(remote_pubkey);
+            if (maybe_token)
             {
                 settings.token = maybe_token->data();
                 settings.tokenlen = maybe_token->size();

src/endpoint.cpp

@@ -407,8 +407,18 @@ namespace oxen::quic
 
         conn.drop_streams();
 
-        conns.erase(rid);
-        log::debug(log_cat, "Deleted connection ({})", rid);
+        if (auto it = conns.find(rid); it != conns.end())
+        {
+            // Defer destruction until the next event loop tick because there are code paths that
+            // can land here from within an ongoing connection method and so it isn't safe to allow
+            // the Connection to get destroyed right now.
+            reset_soon(std::move(it->second));
+            // We do want to remove it from `conns`, though, because some scheduled callbacks check
+            // for `rid` being still in the endpoint and so, in that respect, we want the connection
+            // to be considered gone even if its destructor doesn't fire yet.
+            conns.erase(it);
+            log::debug(log_cat, "Deleted connection ({})", rid);
+        }
     }
 
     int Endpoint::validate_anti_replay(ustring key, ustring data, time_t /* exp */)