Skip to content

Commit

Permalink
Fix heap-use-after-free error when shutting down envoy. (#758)
Browse files Browse the repository at this point in the history
The HotRestart holds a socket event for the domain socket used for admin
purpose, it need to be deleted before the envoy server gets deleted, as
the envoy server own the event_base and which already deleted the
registered events with the event_base_free().
  • Loading branch information
fengli79 authored and mattklein123 committed Apr 13, 2017
1 parent 5b523d2 commit fa579bf
Show file tree
Hide file tree
Showing 7 changed files with 14 additions and 0 deletions.
5 changes: 5 additions & 0 deletions include/envoy/server/hot_restart.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ class HotRestart {
*/
virtual void terminateParent() PURE;

/**
* Shutdown the hot restarter.
*/
virtual void shutdown() PURE;

/**
* Return the hot restart compatability version so that operations code can decide whether to
* perform a full or hot restart.
Expand Down
2 changes: 2 additions & 0 deletions source/exe/hot_restart.cc
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ void HotRestartImpl::terminateParent() {
parent_terminated_ = true;
}

void HotRestartImpl::shutdown() { socket_event_.reset(); }

std::string HotRestartImpl::version() { return SharedMemory::version(); }

} // Server
1 change: 1 addition & 0 deletions source/exe/hot_restart.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class HotRestartImpl : public HotRestart,
void initialize(Event::Dispatcher& dispatcher, Server::Instance& server) override;
void shutdownParentAdmin(ShutdownParentAdminInfo& info) override;
void terminateParent() override;
void shutdown() override;
std::string version() override;

// RawStatDataAllocator
Expand Down
2 changes: 2 additions & 0 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ InstanceImpl::InstanceImpl(Options& options, TestHooks& hooks, HotRestart& resta
}
}

InstanceImpl::~InstanceImpl() { restarter_.shutdown(); }

Upstream::ClusterManager& InstanceImpl::clusterManager() { return config_->clusterManager(); }

Tracing::HttpTracer& InstanceImpl::httpTracer() { return config_->httpTracer(); }
Expand Down
2 changes: 2 additions & 0 deletions source/server/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory,
const LocalInfo::LocalInfo& local_info);

~InstanceImpl() override;

void run();

// Server::Instance
Expand Down
1 change: 1 addition & 0 deletions test/integration/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class TestHotRestart : public HotRestart {
void initialize(Event::Dispatcher&, Server::Instance&) override {}
void shutdownParentAdmin(ShutdownParentAdminInfo&) override {}
void terminateParent() override {}
void shutdown() override {}
std::string version() override { return "1"; }
};

Expand Down
1 change: 1 addition & 0 deletions test/mocks/server/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class MockHotRestart : public HotRestart {
MOCK_METHOD2(initialize, void(Event::Dispatcher& dispatcher, Server::Instance& server));
MOCK_METHOD1(shutdownParentAdmin, void(ShutdownParentAdminInfo& info));
MOCK_METHOD0(terminateParent, void());
MOCK_METHOD0(shutdown, void());
MOCK_METHOD0(version, std::string());
};

Expand Down

0 comments on commit fa579bf

Please sign in to comment.