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 10 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
11 changes: 10 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,16 @@ 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.

@mattklein123 mattklein123 Aug 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@danzh2010 @chadr123 sorry I haven't been tracking this PR. Can you describe in more detail what the actual problem is? I would like to understand this better, which will help build a test, but also make this comment better. My very quick understanding is that this is the line that is crashing? Is that right?

envoy/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc

Line 247 in 8452f4b

const Api::IoCallUint64Result rc = cluster_.filter_.read_callbacks_->udpListener().send(data);

So the issue is a race condition in which the listener is removed, but before we destroy the filter during deferred deletion. Is that right? So the fix here is to make sure that when the listener is shutdown/destroyed the filter is also destroyed?

Yeah it might not be possible to have an integration test for this case but I will determine that once we make this comment more complete in terms of what is going on here. Thank you!

/wait

@chadr123 chadr123 Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. All your understanding is right.
So, the filter should be deleted before listener deletion because the filter refers to the listener.

@mattklein123 mattklein123 Aug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok thanks. I agree I don't see how it will be possible to make an integration test for this. Please make the comment more robust and we can ship. Thank you!

@chadr123 chadr123 Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about this?

Suggested change
// The read_filter_ should be deleted before the udp_listener_ is deleted.
// The read filter should be deleted before the udp listener is deleted.
// The read filter refers to udp listener to send packets to downstream that comes from upstream.
// So, if udp listener is deleted first than read filter, the read filter try to send packets to downstream but there is no udp listener.
// In that case, null pointer referencing will be occurred.
// This is serious in case of hot restart because if previous envoy process is crashed by this null pointer referencing, new envoy process that started for hot restart will be also stopped because it cannot signal to previous envoy process.

@mattklein123 mattklein123 Aug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do this (reflow as appropriate):

// 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.

Thank you!

@chadr123 chadr123 Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. I have changed the comments now. :)

if (read_filter_.get() != nullptr && udp_listener_.get() == nullptr) {
throw Envoy::EnvoyException(

@danzh2010 danzh2010 Jul 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it have to be an exception? If it's just added for the sake of test, I think it's better to written the test in a way that without read_filter_.reset() the test would crash.

@chadr123 chadr123 Jul 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will change that code and the test to as you mentioned. :)

fmt::format("The read_filter should be deleted before the udp_listener is deleted: {} ",
config_->name()));
}
read_filter_.reset();
udp_listener_.reset();
}

// Network::UdpListenerFilterManager
void addReadFilter(Network::UdpListenerReadFilterPtr&& filter) override;
Expand Down
41 changes: 39 additions & 2 deletions test/server/connection_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ namespace Envoy {
namespace Server {
namespace {

class MockUpstreamUdpFilter : public Network::UdpListenerReadFilter {

@danzh2010 danzh2010 Jul 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No interface is mocked. rename it to: UnimplementedUpstreamUdpFilter?

@chadr123 chadr123 Jul 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. It is better to rename it to other one.

public:
MockUpstreamUdpFilter(Network::UdpReadFilterCallbacks& callbacks)
: UdpListenerReadFilter(callbacks) {}

// Network::UdpListenerReadFilter
void onData(Network::UdpRecvData&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
void onReceiveError(Api::IoError::IoErrorCode) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
};

class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<Logger::Id::main> {
public:
ConnectionHandlerTest()
Expand Down Expand Up @@ -138,13 +148,14 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::Loggable<L
std::chrono::milliseconds listener_filters_timeout = std::chrono::milliseconds(15000),
bool continue_on_listener_filters_timeout = false,
std::shared_ptr<NiceMock<Network::MockFilterChainManager>> overridden_filter_chain_manager =
nullptr) {
nullptr,
bool return_null_listener = false) {
listeners_.emplace_back(std::make_unique<TestListener>(
*this, tag, bind_to_port, hand_off_restored_destination_connections, name, socket_type,
listener_filters_timeout, continue_on_listener_filters_timeout, socket_factory_,
overridden_filter_chain_manager));
EXPECT_CALL(*socket_factory_, socketType()).WillOnce(Return(socket_type));
if (listener == nullptr) {
if (listener == nullptr && return_null_listener == false) {
// Expecting listener config in place update.
// If so, dispatcher would not create new network listener.
return listeners_.back().get();
Expand Down Expand Up @@ -1016,6 +1027,32 @@ TEST_F(ConnectionHandlerTest, ListenerFilterWorks) {
EXPECT_CALL(*listener, onDestroy());
}

// The read_filter should be deleted before the udp_listener is deleted.

@danzh2010 danzh2010 Jul 29, 2020

Copy link
Copy Markdown
Contributor

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?

@chadr123 chadr123 Jul 30, 2020

Copy link
Copy Markdown
Contributor Author

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. :(

@danzh2010 danzh2010 Jul 31, 2020

Copy link
Copy Markdown
Contributor

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.

@chadr123 chadr123 Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

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;

TestListener* test_listener = addListener(1, true, false, "test_listener", nullptr, nullptr,
nullptr, nullptr, Network::Socket::Type::Datagram,
std::chrono::milliseconds(), false, nullptr, true);

EXPECT_CALL(factory_, createUdpListenerFilterChain(_, _))
.WillOnce(Invoke([&](Network::UdpListenerFilterManager& udp_listener,
Network::UdpReadFilterCallbacks& callbacks) -> bool {
udp_listener.addReadFilter(std::make_unique<MockUpstreamUdpFilter>(callbacks));
return true;
}));
EXPECT_CALL(*socket_factory_, localAddress()).WillRepeatedly(ReturnRef(local_address_));
handler_->addListener(absl::nullopt, *test_listener);

try {
handler_->stopListeners();
FAIL();
} catch (const Envoy::EnvoyException& e) {
EXPECT_THAT(e.what(),
HasSubstr("The read_filter should be deleted before the udp_listener is deleted"));
}
}

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