From e4ed53224c17606487c6351dc9f6b28385b88916 Mon Sep 17 00:00:00 2001 From: Jeff Becker Date: Fri, 4 Jun 2021 08:34:41 -0400 Subject: [PATCH] use weak_ptr on a path to reference its parent pathset instead of a bare pointer so crashes dont happen --- llarp/exit/session.hpp | 6 +++++ llarp/handlers/null.hpp | 6 +++++ llarp/handlers/tun.hpp | 6 +++++ llarp/path/path.cpp | 42 ++++++++++++++++++++--------- llarp/path/path.hpp | 4 +-- llarp/path/path_context.cpp | 3 ++- llarp/path/pathbuilder.cpp | 2 +- llarp/path/pathbuilder.hpp | 2 +- llarp/path/pathset.hpp | 4 +++ llarp/service/endpoint.cpp | 2 +- llarp/service/outbound_context.cpp | 1 - llarp/service/outbound_context.hpp | 6 +++++ pybind/llarp/handlers/pyhandler.hpp | 6 +++++ test/path/test_path.cpp | 2 +- 14 files changed, 71 insertions(+), 21 deletions(-) diff --git a/llarp/exit/session.hpp b/llarp/exit/session.hpp index 3bea7ac99..def9115ac 100644 --- a/llarp/exit/session.hpp +++ b/llarp/exit/session.hpp @@ -51,6 +51,12 @@ namespace llarp return shared_from_this(); } + std::weak_ptr + GetWeak() override + { + return weak_from_this(); + } + void BlacklistSNode(const RouterID snode) override; diff --git a/llarp/handlers/null.hpp b/llarp/handlers/null.hpp index 853332fbb..e8f209dc6 100644 --- a/llarp/handlers/null.hpp +++ b/llarp/handlers/null.hpp @@ -61,6 +61,12 @@ namespace llarp return shared_from_this(); } + std::weak_ptr + GetWeak() override + { + return weak_from_this(); + } + bool SupportsV6() const override { diff --git a/llarp/handlers/tun.hpp b/llarp/handlers/tun.hpp index 0e614a8f6..29daf8fcc 100644 --- a/llarp/handlers/tun.hpp +++ b/llarp/handlers/tun.hpp @@ -34,6 +34,12 @@ namespace llarp return shared_from_this(); } + std::weak_ptr + GetWeak() override + { + return weak_from_this(); + } + void Thaw() override; diff --git a/llarp/path/path.cpp b/llarp/path/path.cpp index 80c107220..986b5bd23 100644 --- a/llarp/path/path.cpp +++ b/llarp/path/path.cpp @@ -25,10 +25,10 @@ namespace llarp { Path::Path( const std::vector& h, - PathSet* parent, + std::weak_ptr pathset, PathRole startingRoles, std::string shortName) - : m_PathSet(parent), _role(startingRoles), m_shortName(std::move(shortName)) + : m_PathSet{pathset}, _role{startingRoles}, m_shortName{std::move(shortName)} { hops.resize(h.size()); @@ -54,7 +54,7 @@ namespace llarp // initialize parts of the introduction intro.router = hops[hsz - 1].rc.pubkey; intro.pathID = hops[hsz - 1].txID; - if (parent) + if (auto parent = m_PathSet.lock()) EnterState(ePathBuilding, parent->Now()); } @@ -253,7 +253,10 @@ namespace llarp edge = *failedAt; r->loop()->call([r, self = shared_from_this(), edge]() { self->EnterState(ePathFailed, r->Now()); - self->m_PathSet->HandlePathBuildFailedAt(self, edge); + if (auto parent = self->m_PathSet.lock()) + { + parent->HandlePathBuildFailedAt(self, edge); + } }); } @@ -272,7 +275,10 @@ namespace llarp if (st == ePathExpired && _status == ePathBuilding) { _status = st; - m_PathSet->HandlePathBuildTimeout(shared_from_this()); + if (auto parent = m_PathSet.lock()) + { + parent->HandlePathBuildTimeout(shared_from_this()); + } } else if (st == ePathBuilding) { @@ -287,7 +293,10 @@ namespace llarp { LogInfo("path ", Name(), " died"); _status = st; - m_PathSet->HandlePathDied(shared_from_this()); + if (auto parent = m_PathSet.lock()) + { + parent->HandlePathDied(shared_from_this()); + } } else if (st == ePathEstablished && _status == ePathTimeout) { @@ -371,11 +380,14 @@ namespace llarp void Path::Rebuild() { - std::vector newHops; - for (const auto& hop : hops) - newHops.emplace_back(hop.rc); - LogInfo(Name(), " rebuilding on ", ShortName()); - m_PathSet->Build(newHops); + if (auto parent = m_PathSet.lock()) + { + std::vector newHops; + for (const auto& hop : hops) + newHops.emplace_back(hop.rc); + LogInfo(Name(), " rebuilding on ", ShortName()); + parent->Build(newHops); + } } void @@ -681,8 +693,12 @@ namespace llarp bool Path::HandleHiddenServiceFrame(const service::ProtocolFrame& frame) { - MarkActive(m_PathSet->Now()); - return m_DataHandler && m_DataHandler(shared_from_this(), frame); + if (auto parent = m_PathSet.lock()) + { + MarkActive(parent->Now()); + return m_DataHandler && m_DataHandler(shared_from_this(), frame); + } + return false; } template diff --git a/llarp/path/path.hpp b/llarp/path/path.hpp index 44d130a39..f8b88860e 100644 --- a/llarp/path/path.hpp +++ b/llarp/path/path.hpp @@ -87,7 +87,7 @@ namespace llarp HopList hops; - PathSet* const m_PathSet; + std::weak_ptr m_PathSet; service::Introduction intro; @@ -95,7 +95,7 @@ namespace llarp Path( const std::vector& routers, - PathSet* parent, + std::weak_ptr parent, PathRole startingRoles, std::string shortName); diff --git a/llarp/path/path_context.cpp b/llarp/path/path_context.cpp index fe737396e..f3e062fd7 100644 --- a/llarp/path/path_context.cpp +++ b/llarp/path/path_context.cpp @@ -225,7 +225,8 @@ namespace llarp auto itr = map.second.find(id); if (itr != map.second.end()) { - return itr->second->m_PathSet->GetSelf(); + if (auto parent = itr->second->m_PathSet.lock()) + return parent; } return nullptr; } diff --git a/llarp/path/pathbuilder.cpp b/llarp/path/pathbuilder.cpp index f3129ce1f..28c7a226e 100644 --- a/llarp/path/pathbuilder.cpp +++ b/llarp/path/pathbuilder.cpp @@ -434,7 +434,7 @@ namespace llarp ctx->pathset = self; std::string path_shortName = "[path " + m_router->ShortName() + "-"; path_shortName = path_shortName + std::to_string(m_router->NextPathBuildNumber()) + "]"; - auto path = std::make_shared(hops, self.get(), roles, std::move(path_shortName)); + auto path = std::make_shared(hops, GetWeak(), roles, std::move(path_shortName)); LogInfo(Name(), " build ", path->ShortName(), ": ", path->HopsString()); path->SetBuildResultHook([self](Path_ptr p) { self->HandlePathBuilt(p); }); diff --git a/llarp/path/pathbuilder.hpp b/llarp/path/pathbuilder.hpp index 48e72ae77..2114ae2c4 100644 --- a/llarp/path/pathbuilder.hpp +++ b/llarp/path/pathbuilder.hpp @@ -57,7 +57,7 @@ namespace llarp DoPathBuildBackoff(); public: - AbstractRouter* m_router; + AbstractRouter* const m_router; SecretKey enckey; size_t numHops; llarp_time_t lastBuild = 0s; diff --git a/llarp/path/pathset.hpp b/llarp/path/pathset.hpp index 05b5a5e9e..8457441b2 100644 --- a/llarp/path/pathset.hpp +++ b/llarp/path/pathset.hpp @@ -117,6 +117,10 @@ namespace llarp virtual PathSet_ptr GetSelf() = 0; + /// get a weak_ptr of ourself + virtual std::weak_ptr + GetWeak() = 0; + virtual void BuildOne(PathRole roles = ePathRoleAny) = 0; diff --git a/llarp/service/endpoint.cpp b/llarp/service/endpoint.cpp index 0d045e5a1..332d6d01f 100644 --- a/llarp/service/endpoint.cpp +++ b/llarp/service/endpoint.cpp @@ -1447,7 +1447,7 @@ namespace llarp path->Endpoint(), order, GenTXID(), - timeout); + timeout + (2 * path->intro.latency)); LogInfo( "doing lookup for ", remote, diff --git a/llarp/service/outbound_context.cpp b/llarp/service/outbound_context.cpp index 6d7a1dc7d..4e3fab561 100644 --- a/llarp/service/outbound_context.cpp +++ b/llarp/service/outbound_context.cpp @@ -333,7 +333,6 @@ namespace llarp bool OutboundContext::Pump(llarp_time_t now) { - if (ReadyToSend() and remoteIntro.router.IsZero()) { SwapIntros(); diff --git a/llarp/service/outbound_context.hpp b/llarp/service/outbound_context.hpp index e18bb6430..4242e85bc 100644 --- a/llarp/service/outbound_context.hpp +++ b/llarp/service/outbound_context.hpp @@ -38,6 +38,12 @@ namespace llarp return shared_from_this(); } + std::weak_ptr + GetWeak() override + { + return weak_from_this(); + } + Address Addr() const; diff --git a/pybind/llarp/handlers/pyhandler.hpp b/pybind/llarp/handlers/pyhandler.hpp index 19bcc9d05..97fdc9572 100644 --- a/pybind/llarp/handlers/pyhandler.hpp +++ b/pybind/llarp/handlers/pyhandler.hpp @@ -54,6 +54,12 @@ namespace llarp return shared_from_this(); } + std::weak_ptr + GetWeak() override + { + return weak_from_this(); + } + bool SupportsV6() const override { diff --git a/test/path/test_path.cpp b/test/path/test_path.cpp index 309af48f5..d5ad3d56c 100644 --- a/test/path/test_path.cpp +++ b/test/path/test_path.cpp @@ -20,7 +20,7 @@ MakePath(std::vector< char > hops) std::vector< RC_t > pathHops; for(const auto& hop : hops) pathHops.push_back(MakeHop(hop)); - return std::make_shared< Path_t >(pathHops, nullptr, 0, "test"); + return std::make_shared< Path_t >(pathHops, std::weak_ptr{}, 0, "test"); } TEST_CASE("UniqueEndpointSet_t has unique endpoints", "[path]")