From b82d83e2716f78534686c31ec394e8f02d2a5a0d Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Wed, 7 Sep 2022 10:43:53 +0200 Subject: [PATCH] nic_router: handle bad DNS in DHCP ACK gracefully The DHCP client of the NIC router used to end up in an uncaught exception if an IP address in the DNS server option of a DHCP ACK was invalid. This commit makes the 'Dns_server' constructor (where the exception originated from) private and instead introduces a public lambda method 'construct' that calls one lambda argument on success and another on failure. This is also in line with the most recent changes to the 'find_by_*' methods of other classes in the NIC router and contributes to the goal of reducing expensive exception handling. Fixes #4465 --- repos/os/src/server/nic_router/dhcp_server.cc | 19 ++++++++------ repos/os/src/server/nic_router/dns.cc | 6 +---- repos/os/src/server/nic_router/dns.h | 16 ++++++++++-- repos/os/src/server/nic_router/ipv4_config.cc | 19 +++++++++++--- .../src/test/nic_router_dhcp/manager/main.cc | 26 ++++++++++++++----- 5 files changed, 61 insertions(+), 25 deletions(-) diff --git a/repos/os/src/server/nic_router/dhcp_server.cc b/repos/os/src/server/nic_router/dhcp_server.cc index 063e513613..94652e4467 100644 --- a/repos/os/src/server/nic_router/dhcp_server.cc +++ b/repos/os/src/server/nic_router/dhcp_server.cc @@ -34,14 +34,17 @@ Dhcp_server_base::Dhcp_server_base(Xml_node const &node, { node.for_each_sub_node("dns-server", [&] (Xml_node const &sub_node) { - try { - _dns_servers.insert_as_tail(*new (alloc) - Dns_server(sub_node.attribute_value("ip", Ipv4_address()))); - - } catch (Dns_server::Invalid) { - - _invalid(domain, "invalid DNS server entry"); - } + Dns_server::construct( + alloc, sub_node.attribute_value("ip", Ipv4_address { }), + [&] /* handle_success */ (Dns_server &server) + { + _dns_servers.insert_as_tail(server); + }, + [&] /* handle_failure */ () + { + _invalid(domain, "invalid DNS server entry"); + } + ); }); node.with_optional_sub_node("dns-domain", [&] (Xml_node const &sub_node) { xml_node_with_attribute(sub_node, "name", [&] (Xml_attribute const &attr) { diff --git a/repos/os/src/server/nic_router/dns.cc b/repos/os/src/server/nic_router/dns.cc index fae3892868..d4cbe71f83 100644 --- a/repos/os/src/server/nic_router/dns.cc +++ b/repos/os/src/server/nic_router/dns.cc @@ -30,11 +30,7 @@ using namespace Genode; Dns_server::Dns_server(Ipv4_address const &ip) : _ip { ip } -{ - if (!_ip.valid()) { - throw Invalid { }; - } -} +{ } bool Dns_server::equal_to(Dns_server const &server) const diff --git a/repos/os/src/server/nic_router/dns.h b/repos/os/src/server/nic_router/dns.h index 6d747e56a9..2f8fb6e2a8 100644 --- a/repos/os/src/server/nic_router/dns.h +++ b/repos/os/src/server/nic_router/dns.h @@ -43,11 +43,23 @@ class Net::Dns_server : private Genode::Noncopyable, Net::Ipv4_address const _ip; + Dns_server(Net::Ipv4_address const &ip); + public: - struct Invalid : Genode::Exception { }; + template - Dns_server(Net::Ipv4_address const &ip); + static void construct(Genode::Allocator &alloc, + Net::Ipv4_address const &ip, + HANDLE_SUCCESS_FN && handle_success, + HANDLE_FAILURE_FN && handle_failure) + { + if (!ip.valid()) { + handle_failure(); + } + handle_success(*new (alloc) Dns_server(ip)); + } bool equal_to(Dns_server const &server) const; diff --git a/repos/os/src/server/nic_router/ipv4_config.cc b/repos/os/src/server/nic_router/ipv4_config.cc index 22ef3ebf8e..8234e3ebff 100644 --- a/repos/os/src/server/nic_router/ipv4_config.cc +++ b/repos/os/src/server/nic_router/ipv4_config.cc @@ -48,8 +48,14 @@ Ipv4_config::Ipv4_config(Ipv4_config const &ip_config, _gateway { ip_config._gateway } { ip_config.for_each_dns_server([&] (Dns_server const &dns_server) { - _dns_servers.insert_as_tail( - *new (alloc) Dns_server(dns_server.ip())); + Dns_server::construct( + alloc, dns_server.ip(), + [&] /* handle_success */ (Dns_server &server) + { + _dns_servers.insert_as_tail(server); + }, + [&] /* handle_failure */ () { } + ); }); _dns_domain_name.set_to(ip_config.dns_domain_name()); } @@ -69,7 +75,14 @@ Ipv4_config::Ipv4_config(Dhcp_packet &dhcp_ack, dhcp_ack.option() }; dns_server.for_each_address([&] (Ipv4_address const &addr) { - _dns_servers.insert_as_tail(*new (alloc) Dns_server(addr)); + Dns_server::construct( + alloc, addr, + [&] /* handle_success */ (Dns_server &server) + { + _dns_servers.insert_as_tail(server); + }, + [&] /* handle_failure */ () { } + ); }); } catch (Dhcp_packet::Option_not_found) { } diff --git a/repos/os/src/test/nic_router_dhcp/manager/main.cc b/repos/os/src/test/nic_router_dhcp/manager/main.cc index d208f0d9b2..da36bce4f5 100644 --- a/repos/os/src/test/nic_router_dhcp/manager/main.cc +++ b/repos/os/src/test/nic_router_dhcp/manager/main.cc @@ -107,11 +107,17 @@ void Local::Main::_handle_router_state() domain_node.for_each_sub_node( "dns", [&] (Xml_node const &dns_node) - { - - dns_servers.insert_as_tail( - *new (_heap) Dns_server(dns_node.attribute_value("ip", Ipv4_address { }))); - }); + { + Dns_server::construct( + _heap, dns_node.attribute_value("ip", Ipv4_address { }), + [&] /* handle_success */ (Dns_server &server) + { + dns_servers.insert_as_tail(server); + }, + [&] /* handle_failure */ () { } + ); + } + ); /* * If the new list of DNS servers differs from the stored list, @@ -122,8 +128,14 @@ void Local::Main::_handle_router_state() _dns_servers.destroy_each(_heap); dns_servers.for_each([&] (Dns_server const &dns_server) { - _dns_servers.insert_as_tail( - *new (_heap) Dns_server(dns_server.ip())); + Dns_server::construct( + _heap, dns_server.ip(), + [&] /* handle_success */ (Dns_server &server) + { + _dns_servers.insert_as_tail(server); + }, + [&] /* handle_failure */ () { } + ); }); _router_config_outdated = true; }