Skip to content

udp: prevent crashing the envoy if udpListener is empty #11914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 16 commits into from
Aug 6, 2020
Merged

udp: prevent crashing the envoy if udpListener is empty #11914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b256299
udp: prevent crashing the envoy if udpListener is empty
Jul 7, 2020
2982aa4
Fix linux-x64 asan ci
Jul 8, 2020
33dd0de
Remove new interface and change the previous method to return a pointer
Jul 9, 2020
4456676
Add a new interfaces to disable packet processors
Jul 15, 2020
f2c3740
Fix clang_tidy ci checking
Jul 16, 2020
3666790
Fix code coverage ci checking
Jul 16, 2020
5feb051
Kick CI
Jul 17, 2020
9474d9a
Kick CI
Jul 17, 2020
93db712
Delete the read filter when the udp listener is deleted as well
Jul 21, 2020
f02e1d1
Add a regression test
Jul 27, 2020
3a17841
Kick CI
Jul 28, 2020
57a6351
Test actual order of deletion of listener and read filter
Jul 29, 2020
542e065
fix ci
Jul 29, 2020
6e78449
fix format
Jul 29, 2020
f99cd24
fix format
Jul 29, 2020
bd49c08
Change comments for fixing
Aug 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion source/server/connection_handler_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,14 @@ class ActiveRawUdpListener : public Network::UdpListenerCallbacks,
Network::Listener* listener() override { return udp_listener_.get(); }
void pauseListening() override { udp_listener_->disable(); }
void resumeListening() override { udp_listener_->enable(); }
void shutdownListener() override { udp_listener_.reset(); }
void shutdownListener() override {
// The read filter should be deleted before the UDP listener is deleted.
// The read filter refers to the UDP listener to send packets to downstream.
// If the UDP listener is deleted before the read filter, the read filter may try to use it
// after deletion.
read_filter_.reset();
udp_listener_.reset();
}

// Network::UdpListenerFilterManager
void addReadFilter(Network::UdpListenerReadFilterPtr&& filter) override;
Expand Down
62 changes: 62 additions & 0 deletions test/server/connection_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,39 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L

using TestListenerPtr = std::unique_ptr<TestListener>;

class MockUpstreamUdpFilter : public Network::UdpListenerReadFilter {
public:
MockUpstreamUdpFilter(ConnectionHandlerTest& parent, Network::UdpReadFilterCallbacks& callbacks)
: UdpListenerReadFilter(callbacks), parent_(parent) {}
~MockUpstreamUdpFilter() override {
parent_.deleted_before_listener_ = !parent_.udp_listener_deleted_;
}

MOCK_METHOD(void, onData, (Network::UdpRecvData&), (override));
MOCK_METHOD(void, onReceiveError, (Api::IoError::IoErrorCode), (override));

private:
ConnectionHandlerTest& parent_;
};

class MockUpstreamUdpListener : public Network::UdpListener {
public:
explicit MockUpstreamUdpListener(ConnectionHandlerTest& parent) : parent_(parent) {
ON_CALL(*this, dispatcher()).WillByDefault(ReturnRef(dispatcher_));
}
~MockUpstreamUdpListener() override { parent_.udp_listener_deleted_ = true; }

MOCK_METHOD(void, enable, (), (override));
MOCK_METHOD(void, disable, (), (override));
MOCK_METHOD(Event::Dispatcher&, dispatcher, (), (override));
MOCK_METHOD(Network::Address::InstanceConstSharedPtr&, localAddress, (), (const, override));
MOCK_METHOD(Api::IoCallUint64Result, send, (const Network::UdpSendData&), (override));

private:
ConnectionHandlerTest& parent_;
Event::MockDispatcher dispatcher_;
};

TestListener* addListener(
uint64_t tag, bool bind_to_port, bool hand_off_restored_destination_connections,
const std::string& name, Network::Listener* listener,
Expand Down Expand Up @@ -190,6 +223,8 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
NiceMock<Api::MockOsSysCalls> os_sys_calls_;
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{&os_sys_calls_};
std::shared_ptr<NiceMock<Network::MockListenerFilterMatcher>> listener_filter_matcher_;
bool udp_listener_deleted_ = false;
bool deleted_before_listener_ = false;
};

// Verify that if a listener is removed while a rebalanced connection is in flight, we correctly
Expand Down Expand Up @@ -1016,6 +1051,33 @@ TEST_F(ConnectionHandlerTest, ListenerFilterWorks) {
EXPECT_CALL(*listener, onDestroy());
}

// The read_filter should be deleted before the udp_listener is deleted.
Copy link
Contributor

@danzh2010 danzh2010 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a UDP proxy filter test to reproduce the crash you encountered and verify that the crash is gone with the fix?

Copy link
Contributor Author

@chadr123 chadr123 Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the read filter is reset so that any packets cannot be received that send by upstream by the my fix.
I can add a UDP proxy filter test to reproduce the crash I encountered but it is hard to verify that the crash is gone on UDP proxy filter test because there is no framework or method to simulate it.
As you know that it is simulated by call the file_event_cb_ directly.
But if the read filter is reset, the file event callback that the read filter have is never cannot be called.

Do you have any ideas to simulate without call the file_event_cb_ directly on the UDP proxy filter test?
I have no ideas for that. :(

Copy link
Contributor

@danzh2010 danzh2010 Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about any integration test which don't simulate file_event_cb_ but actually sends packet to that socket? @mattklein123 about ideas of how to write regression test for this crash as Matt knows better about that test code.

Copy link
Contributor Author

@chadr123 chadr123 Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the integration test but I cannot find a way to test my patch.
As you know that the root cause was race condition between read filter and udp listener.
So, if we want to test the before and after, we should control the deletion of read filter and udp listener.
But I cannot find the framework or a way for that. :(
That is way I check the order of read filter and udp listener deletions.
Do I have to really add more tests for this patch?

TEST_F(ConnectionHandlerTest, ShutdownUdpListener) {
InSequence s;

Network::MockUdpReadFilterCallbacks dummy_callbacks;
auto listener = new NiceMock<MockUpstreamUdpListener>(*this);
TestListener* test_listener =
addListener(1, true, false, "test_listener", listener, nullptr, nullptr, nullptr,
Network::Socket::Type::Datagram, std::chrono::milliseconds(), false, nullptr);
auto filter = std::make_unique<NiceMock<MockUpstreamUdpFilter>>(*this, dummy_callbacks);

EXPECT_CALL(factory_, createUdpListenerFilterChain(_, _))
.WillOnce(Invoke([&](Network::UdpListenerFilterManager& udp_listener,
Network::UdpReadFilterCallbacks&) -> bool {
udp_listener.addReadFilter(std::move(filter));
return true;
}));
EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(local_address_));
EXPECT_CALL(dummy_callbacks.udp_listener_, onDestroy());

handler_->addListener(absl::nullopt, *test_listener);
handler_->stopListeners();

ASSERT_TRUE(deleted_before_listener_)
<< "The read_filter_ should be deleted before the udp_listener_ is deleted.";
}

} // namespace
} // namespace Server
} // namespace Envoy