From e254c1220c641a8cc5363a902729b0d5f0199aca Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 18:39:23 +0200 Subject: [PATCH 1/9] libndt: simpler-to-override resolver Overriding getaddrinfo() seems a bit complex. We need to do that in MK to use other resolvers. So, how about splitting connect_tcp() such that name resolution fills a vector of addresses, using getaddrinfo() by default, and then connect_tcp() iterates over such vector using getaddrinfo() in a simpler way? With this change, in MK we need to override a function that takes in input a domain name (or IP address) and outputs the corresponding list of IP addresses (or just the same IP). This is simpler to implement with various DNS libraries than to write a full fledged replacement for the whole complexity of getaddrinfo(). --- libndt.cpp | 112 +++++++++++++++++++++++++++++++++++++++-------------- libndt.hpp | 13 ++++++- 2 files changed, 94 insertions(+), 31 deletions(-) diff --git a/libndt.cpp b/libndt.cpp index 70b0935..5a150b1 100644 --- a/libndt.cpp +++ b/libndt.cpp @@ -719,41 +719,49 @@ bool Client::connect_tcp(const std::string &hostname, const std::string &port, EMIT_WARNING("socket already connected"); return false; } - addrinfo hints{}; - hints.ai_socktype = SOCK_STREAM; - hints.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV; - addrinfo *rp = nullptr; - int rv = this->getaddrinfo(hostname.data(), port.data(), &hints, &rp); - if (rv != 0) { - hints.ai_flags &= ~AI_NUMERICHOST; - rv = this->getaddrinfo(hostname.data(), port.data(), &hints, &rp); + // Implementation note: we could perform getaddrinfo() in one pass but having + // a virtual API that resolves a hostname to a vector of IP addresses makes + // life easier when you want to override hostname resolution, because you have + // to reimplement a simpler method, compared to reimplementing getaddrinfo(). + std::vector addresses; + if (!resolve(hostname, &addresses)) { + return false; + } + for (auto &addr : addresses) { + addrinfo hints{}; + hints.ai_socktype = SOCK_STREAM; + hints.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV; + addrinfo *rp = nullptr; + int rv = this->getaddrinfo(addr.data(), port.data(), &hints, &rp); if (rv != 0) { - EMIT_WARNING("getaddrinfo() failed: " << gai_strerror(rv)); + EMIT_WARNING("unexpected getaddrinfo() failure"); return false; } - // FALLTHROUGH - } - EMIT_DEBUG("getaddrinfo(): okay"); - for (auto aip = rp; (aip); aip = aip->ai_next) { - *sock = this->socket(aip->ai_family, aip->ai_socktype, 0); - if (*sock == -1) { - EMIT_WARNING("socket() failed: " << get_last_error()); - continue; + assert(rp); + for (auto aip = rp; (aip); aip = aip->ai_next) { + *sock = this->socket(aip->ai_family, aip->ai_socktype, 0); + if (*sock == -1) { + EMIT_WARNING("socket() failed: " << get_last_error()); + continue; + } + // The following two lines ensure that casting `size_t` to + // SockLen is safe because SockLen is `int` and the value of + // the ai_addrlen field is always small enough. + static_assert(sizeof(SockLen) == sizeof(int), "Wrong SockLen size"); + assert(aip->ai_addrlen <= INT_MAX); + if (this->connect(*sock, aip->ai_addr, (SockLen)aip->ai_addrlen) == 0) { + EMIT_DEBUG("connect(): okay"); + break; + } + EMIT_WARNING("connect() failed: " << get_last_error()); + this->closesocket(*sock); + *sock = -1; } - // The following two lines ensure that casting `size_t` to - // SockLen is safe because SockLen is `int` and the value of - // the ai_addrlen field is always small enough. - static_assert(sizeof(SockLen) == sizeof(int), "Wrong SockLen size"); - assert(aip->ai_addrlen <= INT_MAX); - if (this->connect(*sock, aip->ai_addr, (SockLen)aip->ai_addrlen) == 0) { - EMIT_DEBUG("connect(): okay"); - break; + this->freeaddrinfo(rp); + if (*sock != -1) { + break; // we have a connection! } - EMIT_WARNING("connect() failed: " << get_last_error()); - this->closesocket(*sock); - *sock = -1; } - this->freeaddrinfo(rp); return *sock != -1; } @@ -1030,6 +1038,46 @@ bool Client::msg_read_legacy(uint8_t *code, std::string *msg) noexcept { return true; } +// Utilities for low-level + +bool Client::resolve(const std::string &hostname, + std::vector *addrs) noexcept { + assert(addrs != nullptr); + EMIT_DEBUG("resolve: " << hostname); + addrinfo hints{}; + hints.ai_socktype = SOCK_STREAM; + hints.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV; + addrinfo *rp = nullptr; + constexpr const char *portno = "80"; // any port would do + int rv = this->getaddrinfo(hostname.data(), portno, &hints, &rp); + if (rv != 0) { + hints.ai_flags &= ~AI_NUMERICHOST; + rv = this->getaddrinfo(hostname.data(), portno, &hints, &rp); + if (rv != 0) { + EMIT_WARNING("getaddrinfo() failed: " << gai_strerror(rv)); + return false; + } + // FALLTHROUGH + } + assert(rp); + EMIT_DEBUG("getaddrinfo(): okay"); + auto result = true; + for (auto aip = rp; (aip); aip = aip->ai_next) { + char address[NI_MAXHOST], port[NI_MAXSERV]; + if (this->getnameinfo(aip->ai_addr, aip->ai_addrlen, address, + sizeof(address), port, sizeof(port), + NI_NUMERICHOST | NI_NUMERICSERV) != 0) { + EMIT_WARNING("unexpected getnameinfo() failure"); + result = false; + break; + } + addrs->push_back(address); // we only care about address + EMIT_DEBUG("- " << address); + } + this->freeaddrinfo(rp); + return result; +} + // Dependencies (curl) bool Client::query_mlabns_curl(const std::string &url, long timeout, @@ -1085,6 +1133,12 @@ int Client::getaddrinfo(const char *domain, const char *port, return ::getaddrinfo(domain, port, hints, res); } +int Client::getnameinfo(const sockaddr *sa, SockLen salen, char *host, + SockLen hostlen, char *serv, SockLen servlen, + int flags) noexcept { + return ::getnameinfo(sa, salen, host, hostlen, serv, servlen, flags); +} + void Client::freeaddrinfo(addrinfo *aip) noexcept { ::freeaddrinfo(aip); } Socket Client::socket(int domain, int type, int protocol) noexcept { diff --git a/libndt.hpp b/libndt.hpp index 6c29a84..aed71a7 100644 --- a/libndt.hpp +++ b/libndt.hpp @@ -17,6 +17,7 @@ #include #include #include +#include struct addrinfo; struct sockaddr; @@ -26,8 +27,8 @@ namespace measurement_kit { namespace libndt { constexpr uint64_t api_major = 0; -constexpr uint64_t api_minor = 20; -constexpr uint64_t api_patch = 2; +constexpr uint64_t api_minor = 21; +constexpr uint64_t api_patch = 0; constexpr uint8_t nettest_middlebox = 1 << 0; constexpr uint8_t nettest_upload = 1 << 1; @@ -157,6 +158,11 @@ class Client { virtual bool msg_read_legacy(uint8_t *code, std::string *msg) noexcept; + // Utilities for low-level + + virtual bool resolve(const std::string &hostname, + std::vector *addrs) noexcept; + // Dependencies (cURL) virtual bool query_mlabns_curl(const std::string &url, long timeout, @@ -169,6 +175,9 @@ class Client { virtual int getaddrinfo(const char *domain, const char *port, const addrinfo *hints, addrinfo **res) noexcept; + virtual int getnameinfo(const sockaddr *sa, SockLen salen, char *host, + SockLen hostlen, char *serv, SockLen servlen, + int flags) noexcept; virtual void freeaddrinfo(addrinfo *aip) noexcept; virtual Socket socket(int domain, int type, int protocol) noexcept; From 13f2ec94f7c4538513b88ea19612bbd2e7d7f40b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 18:50:37 +0200 Subject: [PATCH 2/9] libndt.cpp: fix compiler warning for Win64 --- libndt.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libndt.cpp b/libndt.cpp index 5a150b1..5be980c 100644 --- a/libndt.cpp +++ b/libndt.cpp @@ -1064,8 +1064,13 @@ bool Client::resolve(const std::string &hostname, auto result = true; for (auto aip = rp; (aip); aip = aip->ai_next) { char address[NI_MAXHOST], port[NI_MAXSERV]; + // The following two lines ensure that casting `size_t` to + // SockLen is safe because SockLen is `int` and the value of + // the ai_addrlen field is always small enough. + static_assert(sizeof(SockLen) == sizeof(int), "Wrong SockLen size"); + assert(sizeof(address) <= INT_MAX && sizeof(port) <= INT_MAX); if (this->getnameinfo(aip->ai_addr, aip->ai_addrlen, address, - sizeof(address), port, sizeof(port), + (SockLen)sizeof(address), port, (SockLen)sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV) != 0) { EMIT_WARNING("unexpected getnameinfo() failure"); result = false; From 01b738c3bcde476e013199a96eb81b93e6df0a30 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:10:44 +0200 Subject: [PATCH 3/9] libndt_test.cpp: reorganize after resolve() --- libndt_test.cpp | 61 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/libndt_test.cpp b/libndt_test.cpp index 6a89e2d..78308c3 100644 --- a/libndt_test.cpp +++ b/libndt_test.cpp @@ -1167,9 +1167,31 @@ TEST_CASE("Client::connect_tcp() requires initial socket to be -1") { REQUIRE(client.connect_tcp("1.2.3.4", "33", &sock) == false); } -class FailGetaddrinfo : public libndt::Client { +class FailResolve : public libndt::Client { public: using libndt::Client::Client; + bool resolve(const std::string &, + std::vector *) noexcept override { + return false; + } +}; + +TEST_CASE("Client::connect_tcp() deals with Client::resolve() failure") { + FailResolve client; + libndt::Socket sock = -1; + client.settings.verbosity = libndt::verbosity_quiet; + REQUIRE(client.connect_tcp("1.2.3.4", "33", &sock) == false); +} + +class FailGetaddrinfoInConnectTcp : public libndt::Client { + public: + using libndt::Client::Client; + bool resolve(const std::string &str, + std::vector *addrs) noexcept override { + REQUIRE(str == "1.2.3.4"); // make sure it did not change + addrs->push_back(str); + return true; + } int getaddrinfo(const char *, const char *, const addrinfo *, addrinfo **) noexcept override { return EAI_AGAIN; @@ -1177,7 +1199,7 @@ class FailGetaddrinfo : public libndt::Client { }; TEST_CASE("Client::connect_tcp() deals with Client::getaddrinfo() failure") { - FailGetaddrinfo client; + FailGetaddrinfoInConnectTcp client; libndt::Socket sock = -1; client.settings.verbosity = libndt::verbosity_quiet; REQUIRE(client.connect_tcp("1.2.3.4", "33", &sock) == false); @@ -1587,6 +1609,41 @@ TEST_CASE( REQUIRE(client.msg_read_legacy(&code, &s) == false); } +// Client::resolve() tests +// ----------------------- + +class FailGetaddrinfo : public libndt::Client { + public: + using libndt::Client::Client; + int getaddrinfo(const char *, const char *, const addrinfo *, + addrinfo **) noexcept override { + return EAI_AGAIN; + } +}; + +TEST_CASE("Client::resolve() deals with Client::getaddrinfo() failure") { + FailGetaddrinfo client; + std::vector addrs; + client.settings.verbosity = libndt::verbosity_quiet; + REQUIRE(client.resolve("x.org", &addrs) == false); +} + +class FailGetnameinfo : public libndt::Client { + public: + using libndt::Client::Client; + int getnameinfo(const sockaddr *, libndt::SockLen, char *, libndt::SockLen, + char *, libndt::SockLen, int) noexcept override { + return EAI_AGAIN; + } +}; + +TEST_CASE("Client::resolve() deals with Client::getnameinfo() failure") { + FailGetnameinfo client; + std::vector addrs; + client.settings.verbosity = libndt::verbosity_quiet; + REQUIRE(client.resolve("x.org", &addrs) == false); +} + // Client::query_mlabns_curl() tests // --------------------------------- From 4ac07d4cfec4c53a2054d67406e4533fdc1755e9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:15:20 +0200 Subject: [PATCH 4/9] appveyor.yml: treat warnings as errors --- appveyor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index dc08ba2..4cce2d7 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -7,5 +7,5 @@ environment: build_script: - cmd: cmake "-G%CMAKE_GENERATOR%" . - - cmd: cmake --build . -- /nologo /property:Configuration=Release + - cmd: cmake --build . -- /nologo /property:Configuration=Release /property:TreatWarningsAsErrors=true - cmd: ctest --output-on-failure -C Release -a From 35639f501e03af85aa0a9488474a6bba87a6f7d8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:18:31 +0200 Subject: [PATCH 5/9] Revert "appveyor.yml: treat warnings as errors" This reverts commit 4ac07d4cfec4c53a2054d67406e4533fdc1755e9. --- appveyor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index 4cce2d7..dc08ba2 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -7,5 +7,5 @@ environment: build_script: - cmd: cmake "-G%CMAKE_GENERATOR%" . - - cmd: cmake --build . -- /nologo /property:Configuration=Release /property:TreatWarningsAsErrors=true + - cmd: cmake --build . -- /nologo /property:Configuration=Release - cmd: ctest --output-on-failure -C Release -a From 9d16b52dda13be0ded5b6b5be72d60391da55a9c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:19:18 +0200 Subject: [PATCH 6/9] CMakeLists.txt: treat warnings as errors This only covers compiler warnings. Links warnings are for now allowed. --- CMakeLists.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index b1f9b21..241654f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,6 +92,9 @@ if("${UNIX}" OR "${MINGW}") # for GCC, -Wmissing-prototypes only works for C/ObjC. set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wmissing-prototypes") endif() +elseif("${MSVC}") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /WX") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /WX") endif() if(${WIN32}) From 89e69547285d8dbb8767edfe9f7f1b6c8161fcf9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:27:37 +0200 Subject: [PATCH 7/9] libndt.cpp: finish fixing MSVC warnings --- libndt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libndt.cpp b/libndt.cpp index 5be980c..ec12a58 100644 --- a/libndt.cpp +++ b/libndt.cpp @@ -1069,7 +1069,7 @@ bool Client::resolve(const std::string &hostname, // the ai_addrlen field is always small enough. static_assert(sizeof(SockLen) == sizeof(int), "Wrong SockLen size"); assert(sizeof(address) <= INT_MAX && sizeof(port) <= INT_MAX); - if (this->getnameinfo(aip->ai_addr, aip->ai_addrlen, address, + if (this->getnameinfo(aip->ai_addr, (SockLen)aip->ai_addrlen, address, (SockLen)sizeof(address), port, (SockLen)sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV) != 0) { EMIT_WARNING("unexpected getnameinfo() failure"); From a1f2f0c0f6334a505bdf0d570a98a0040e780e5f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:27:58 +0200 Subject: [PATCH 8/9] .gitignore: ignore .vs directory --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 2e53a40..193d406 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ /.ninja_deps /.ninja_log +/.vs /CMakeCache.txt /CMakeFiles/ /CTestTestfile.cmake From 60210b60208c890793d026074eac813f081895a9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 13 May 2018 19:32:42 +0200 Subject: [PATCH 9/9] [ci skip] CMakeLists.txt: note possible improvements --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 241654f..029503b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -93,6 +93,7 @@ if("${UNIX}" OR "${MINGW}") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wmissing-prototypes") endif() elseif("${MSVC}") + # TODO(bassosimone): extend this also to linker warnings. set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /WX") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /WX") endif()