Skip to content
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.

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

exception: make Ipv6Instance and Ipv4Instance not throw and remove some try catch pattern #16122

Merged
merged 35 commits into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 9 additions & 0 deletions source/common/network/address_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ StatusOr<Address::InstanceConstSharedPtr> addressFromSockAddr(const sockaddr_sto
NOT_REACHED_GCOVR_EXCL_LINE;
}

Address::InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss,
socklen_t ss_len, bool v6only) {
StatusOr<InstanceConstSharedPtr> address = addressFromSockAddr(ss, ss_len, v6only);
Copy link
Contributor

Choose a reason for hiding this comment

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

add ASSERT(!isMainThread)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean ASSERT(isMainThread)?

Copy link
Member Author

Choose a reason for hiding this comment

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

But this is also called in worker thread. We can use OrDie version in worker thread everywhere, but that would change the behavior of envoy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, yes. I meant ASSERT(isMainThread()). We should only be calling something that can throw if we are in the main thread, right?

Now, if this is used in worker thread, it should be a version that cannot throw, rather than this one. That's the main point of this change, right? We may have to propagate errors up stack and change some call-sites.

Copy link
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 comment like:

Note that a throw may in theory occur in a worker thread, as long as it is not caught (and therefore crashes), and we use macros and lint to avoid catching exceptions in the main thread. In practice we do not expect addresses to be validated in a main-thread flow, and thus never reach worker threads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, comment added.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing something here, but it seems we're not improving anything if we're still either throwing or dying on the working thread?

Can you maybe explain the context of this PR so I can understand scope better. I.e. what's the long term game plan here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a lot less throwing and catching done in worker threads now. The only exception I think is the catch in place needed to deal with unexpected issues in the fuzz test.

if (!address.ok()) {
throw EnvoyException(address.status().ToString());
}
return *address;
}

Address::InstanceConstSharedPtr
getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd, bool v6only) {
chaoqin-li1123 marked this conversation as resolved.
Show resolved Hide resolved
// Set v6only to false so that mapped-v6 address can be normalize to v4
Expand Down
4 changes: 4 additions & 0 deletions source/common/network/address_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ namespace Address {
*/
StatusOr<InstanceConstSharedPtr> addressFromSockAddr(const sockaddr_storage& ss, socklen_t len,
chaoqin-li1123 marked this conversation as resolved.
Show resolved Hide resolved
bool v6only = true);
InstanceConstSharedPtr addressFromSockAddrOrThrow(const sockaddr_storage& ss, socklen_t len,
bool v6only = true);

Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss,
socklen_t ss_len, os_fd_t fd,
Expand Down Expand Up @@ -135,6 +137,7 @@ class Ipv4Instance : public InstanceBase {
private:
/**
* Construct from an existing unix IPv4 socket address (IP v4 address and port).
* Store the status code in passed in parameter instead of throwing.
*/
explicit Ipv4Instance(absl::Status& error, const sockaddr_in* address,
const SocketInterface* sock_interface = nullptr);
Expand Down Expand Up @@ -213,6 +216,7 @@ class Ipv6Instance : public InstanceBase {
private:
/**
* Construct from an existing unix IPv6 socket address (IP v6 address and port).
* Store the status code in passed in parameter instead of throwing.
*/
Ipv6Instance(absl::Status& error, const sockaddr_in6& address, bool v6only = true,
const SocketInterface* sock_interface = nullptr);
Expand Down
4 changes: 2 additions & 2 deletions source/common/network/io_socket_handle_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::localAddress() {
throw EnvoyException(fmt::format("getsockname failed for '{}': ({}) {}", fd_, result.errno_,
errorDetails(result.errno_)));
}
return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, socket_v6only_);
return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_);
}

Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() {
Expand All @@ -611,7 +611,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() {
fmt::format("getsockname failed for '{}': {}", fd_, errorDetails(result.errno_)));
}
}
return Address::getAddressFromSockAddrOrDie(ss, ss_len, -1, true);
return Address::addressFromSockAddrOrThrow(ss, ss_len, socket_v6only_);
}

void IoSocketHandleImpl::initializeFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb,
Expand Down
15 changes: 7 additions & 8 deletions source/common/network/tcp_listener_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,13 @@ void TcpListenerImpl::onSocketEvent(short flags) {
// Pass the 'v6only' parameter as true if the local_address is an IPv6 address. This has no
// effect if the socket is a v4 socket, but for v6 sockets this will create an IPv4 remote
// address if an IPv4 local_address was created from an IPv6 mapped IPv4 address.
Address::InstanceConstSharedPtr remote_address;
if (remote_addr.ss_family == AF_UNIX) {
remote_address = io_handle->peerAddress();
} else {
remote_address = Address::getAddressFromSockAddrOrDie(remote_addr, remote_addr_len, -1,
local_address->ip()->version() ==
Address::IpVersion::v6);
}

const Address::InstanceConstSharedPtr remote_address =
(remote_addr.ss_family == AF_UNIX)
? io_handle->peerAddress()
: Address::addressFromSockAddrOrThrow(remote_addr, remote_addr_len,
local_address->ip()->version() ==
Address::IpVersion::v6);

cb_.onAccept(
std::make_unique<AcceptedSocketImpl>(std::move(io_handle), local_address, remote_address));
Expand Down
34 changes: 26 additions & 8 deletions source/common/network/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,28 @@ uint32_t Utility::portFromUdpUrl(const std::string& url) {

Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std::string& ip_address,
uint16_t port, bool v6only) {
Address::InstanceConstSharedPtr address;
sockaddr_in sa4;
if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) {
sa4.sin_family = AF_INET;
sa4.sin_port = htons(port);
return Address::InstanceFactory::createInstancePtrOrDie<Address::Ipv4Instance>(&sa4);
StatusOr<Address::InstanceConstSharedPtr> address =
Address::InstanceFactory::createInstancePtr<Address::Ipv4Instance>(&sa4);
chaoqin-li1123 marked this conversation as resolved.
Show resolved Hide resolved
if (address.ok()) {
return *address;
}
return nullptr;
}
sockaddr_in6 sa6;
memset(&sa6, 0, sizeof(sa6));
if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) {
sa6.sin6_family = AF_INET6;
sa6.sin6_port = htons(port);
return Address::InstanceFactory::createInstancePtrOrDie<Address::Ipv6Instance>(sa6, v6only);
StatusOr<Address::InstanceConstSharedPtr> address =
Address::InstanceFactory::createInstancePtr<Address::Ipv6Instance>(sa6, v6only);
if (address.ok()) {
return *address;
}
return nullptr;
}
return nullptr;
}
Expand Down Expand Up @@ -180,7 +189,12 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool
}
sa6.sin6_family = AF_INET6;
sa6.sin6_port = htons(port64);
return Address::InstanceFactory::createInstancePtrOrDie<Address::Ipv6Instance>(sa6, v6only);
StatusOr<Address::InstanceConstSharedPtr> address =
Address::InstanceFactory::createInstancePtr<Address::Ipv6Instance>(sa6, v6only);
if (address.ok()) {
return *address;
}
return nullptr;
}
// Treat it as an IPv4 address followed by a port.
const auto pos = ip_address.rfind(':');
Expand All @@ -200,7 +214,12 @@ Utility::parseInternetAddressAndPortNoThrow(const std::string& ip_address, bool
}
sa4.sin_family = AF_INET;
sa4.sin_port = htons(port64);
return Address::InstanceFactory::createInstancePtrOrDie<Address::Ipv4Instance>(&sa4);
StatusOr<Address::InstanceConstSharedPtr> address =
Address::InstanceFactory::createInstancePtrOrDie<Address::Ipv4Instance>(&sa4);
if (address.ok()) {
return *address;
}
chaoqin-li1123 marked this conversation as resolved.
Show resolved Hide resolved
return nullptr;
}

Address::InstanceConstSharedPtr Utility::parseInternetAddressAndPort(const std::string& ip_address,
Expand Down Expand Up @@ -248,9 +267,8 @@ Address::InstanceConstSharedPtr Utility::getLocalAddress(const Address::IpVersio
(ifa->ifa_addr->sa_family == AF_INET6 && version == Address::IpVersion::v6)) {
const struct sockaddr_storage* addr =
reinterpret_cast<const struct sockaddr_storage*>(ifa->ifa_addr);
ret = Address::getAddressFromSockAddrOrDie(
*addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6),
-1, true);
ret = Address::addressFromSockAddrOrThrow(
*addr, (version == Address::IpVersion::v4) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6));
if (!isLoopbackAddress(*ret)) {
break;
}
Expand Down
16 changes: 8 additions & 8 deletions source/common/quic/envoy_quic_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ namespace Quic {
// the heap allocation is too expensive.
Network::Address::InstanceConstSharedPtr
quicAddressToEnvoyAddressInstance(const quic::QuicSocketAddress& quic_address) {
if (!quic_address.IsInitialized()) {
return nullptr;
}
return Network::Address::getAddressFromSockAddrOrDie(
quic_address.generic_address(),
quic_address.host().address_family() == quic::IpAddressFamily::IP_V4 ? sizeof(sockaddr_in)
: sizeof(sockaddr_in6),
-1, false);
return quic_address.IsInitialized()
? Network::Address::addressFromSockAddrOrThrow(quic_address.generic_address(),
quic_address.host().address_family() ==
quic::IpAddressFamily::IP_V4
? sizeof(sockaddr_in)
: sizeof(sockaddr_in6),
false)
: nullptr;
}

quic::QuicSocketAddress envoyIpAddressToQuicSocketAddress(const Network::Address::Ip* envoy_ip) {
Expand Down
6 changes: 2 additions & 4 deletions source/common/upstream/strict_dns_cluster.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,10 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
// make a new address that has port in it. We need to both support IPv6 as well as
// potentially move port handling into the DNS interface itself, which would work better
// for SRV.

ASSERT(resp.address_ != nullptr);
Network::Address::InstanceConstSharedPtr new_address =
Network::Utility::getAddressWithPort(*(resp.address_), port_);
new_hosts.emplace_back(new HostImpl(
parent_.info_, hostname_, new_address,
parent_.info_, hostname_,
Network::Utility::getAddressWithPort(*(resp.address_), port_),
// TODO(zyfjeff): Created through metadata shared pool
std::make_shared<const envoy::config::core::v3::Metadata>(lb_endpoint_.metadata()),
lb_endpoint_.load_balancing_weight().value(), locality_lb_endpoints_.locality(),
Expand Down
1 change: 0 additions & 1 deletion test/common/network/dns_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,6 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) {
// If that flag needs to be set, or c-ares changes its handling this test will need to be updated
// to create another condition where c-ares invokes onAresGetAddrInfoCallback with status ==
// ARES_ECONNREFUSED.

EXPECT_TRUE(peer_->isChannelDirty());

server_->setRefused(false);
Expand Down