Pass IPv6 vs IPv4 down to the shim
The runner currently tries to listen on IPv6 and then falls back to IPv4
on error. The shim does the same. If they pick different ones, this
breaks down.
Normally, fallback happens because the system doesn't have IPv6, and
both sides will make the same decision. But if binding to IPv6 fails for
other reasons, they may mismatch. We're observing them fail due to what
seems to port exhaustion. When this happens, shim and runner don't
agree on the same address family.
Instead, just tell the shim which address to connect to. This doesn't
fix the underlying port exhaustion problem, but it does seem to fix the
flakes. Although given we are still exhausting ports and falling back to
IPv4, it doesn't truly fix it. Later CLs will address port exhaustion by
using a single server port.
This changes the runner <-> shim protocol, but hopefully in a fairly
obvious way that others using BoGo can easily follow. It shouldn't break
our cross-version tests because we keep runner and shim at the same
versio there.
To avoid needing to make an incompatible change to the shim <->
handshaker protocol, which would impact our cross-version tests, this
introduces a mechanism for the shim omit flags when talking to the
handshaker. The handshaker doesn't need to know how to connect to the
runner.
Also print the error string on Windows. Sadly this is a bit tedious.
Change-Id: Ic8bda9a854a115c206c05a659a2e34f544b844a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60885
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/test/PORTING.md b/ssl/test/PORTING.md
index 86ad24d..5819b49 100644
--- a/ssl/test/PORTING.md
+++ b/ssl/test/PORTING.md
@@ -20,14 +20,16 @@
“shim”: a command line program which encapsulates the stack. By
default, the shim points to the BoringSSL shim in the same source
tree, but any program can be supplied via the `-shim-path` flag. The
-runner opens up a server socket and provides the shim with a `-port`
-argument that points to that socket. The shim always connects to the
-runner as a TCP client even when acting as a TLS server. For DTLS,
-there is a small framing layer that gives packet boundaries over
-TCP. The shim can also pass a variety of command line arguments which
-are used to configure the stack under test. These can be found at
-`test_config.cc`.
+runner opens up a server socket and provides the shim with `-port` and
+optional `-ipv6` arguments.
+The shim should connect to loopback as a TCP client on the specified
+port, using IPv6 if `-ipv6` is specified and IPv4 otherwise. This is
+true even when the shim is speaking DTLS or acting as a TLS server.
+For DTLS, there is a small framing layer that gives packet boundaries
+over TCP. The shim can also pass a variety of command line arguments
+which are used to configure the stack under test. These can be found at
+`test_config.cc`.
The shim reports success by exiting with a `0` error code and failure by
reporting a non-zero error code and generally sending a textual error
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index e2f79d3..c14d7c0 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -77,7 +77,20 @@
}
#else
static void PrintSocketError(const char *func) {
- fprintf(stderr, "%s: %d\n", func, WSAGetLastError());
+ int error = WSAGetLastError();
+ char *buffer;
+ DWORD len = FormatMessageA(
+ FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER, 0, error, 0,
+ reinterpret_cast<char *>(&buffer), 0, nullptr);
+ std::string msg = "unknown error";
+ if (len > 0) {
+ msg.assign(buffer, len);
+ while (!msg.empty() && (msg.back() == '\r' || msg.back() == '\n')) {
+ msg.resize(msg.size() - 1);
+ }
+ }
+ LocalFree(buffer);
+ fprintf(stderr, "%s: %s (%d)\n", func, msg.c_str(), error);
}
#endif
@@ -93,56 +106,55 @@
}
};
-// Connect returns a new socket connected to localhost on |port| or -1 on
-// error.
-static int Connect(uint16_t port) {
- for (int af : { AF_INET6, AF_INET }) {
- int sock = socket(af, SOCK_STREAM, 0);
- if (sock == -1) {
- PrintSocketError("socket");
+// Connect returns a new socket connected to the runner, or -1 on error.
+static int Connect(const TestConfig *config) {
+ sockaddr_storage addr;
+ socklen_t addr_len = 0;
+ if (config->ipv6) {
+ sockaddr_in6 sin6;
+ OPENSSL_memset(&sin6, 0, sizeof(sin6));
+ sin6.sin6_family = AF_INET6;
+ sin6.sin6_port = htons(config->port);
+ if (!inet_pton(AF_INET6, "::1", &sin6.sin6_addr)) {
+ PrintSocketError("inet_pton");
return -1;
}
- int nodelay = 1;
- if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
- reinterpret_cast<const char*>(&nodelay), sizeof(nodelay)) != 0) {
- PrintSocketError("setsockopt");
- closesocket(sock);
+ addr_len = sizeof(sin6);
+ memcpy(&addr, &sin6, addr_len);
+ } else {
+ sockaddr_in sin;
+ OPENSSL_memset(&sin, 0, sizeof(sin));
+ sin.sin_family = AF_INET;
+ sin.sin_port = htons(config->port);
+ if (!inet_pton(AF_INET, "127.0.0.1", &sin.sin_addr)) {
+ PrintSocketError("inet_pton");
return -1;
}
-
- sockaddr_storage ss;
- OPENSSL_memset(&ss, 0, sizeof(ss));
- ss.ss_family = af;
- socklen_t len = 0;
-
- if (af == AF_INET6) {
- sockaddr_in6 *sin6 = (sockaddr_in6 *) &ss;
- len = sizeof(*sin6);
- sin6->sin6_port = htons(port);
- if (!inet_pton(AF_INET6, "::1", &sin6->sin6_addr)) {
- PrintSocketError("inet_pton");
- closesocket(sock);
- return -1;
- }
- } else if (af == AF_INET) {
- sockaddr_in *sin = (sockaddr_in *) &ss;
- len = sizeof(*sin);
- sin->sin_port = htons(port);
- if (!inet_pton(AF_INET, "127.0.0.1", &sin->sin_addr)) {
- PrintSocketError("inet_pton");
- closesocket(sock);
- return -1;
- }
- }
-
- if (connect(sock, reinterpret_cast<const sockaddr*>(&ss), len) == 0) {
- return sock;
- }
- closesocket(sock);
+ addr_len = sizeof(sin);
+ memcpy(&addr, &sin, addr_len);
}
- PrintSocketError("connect");
- return -1;
+ int sock = socket(addr.ss_family, SOCK_STREAM, 0);
+ if (sock == -1) {
+ PrintSocketError("socket");
+ return -1;
+ }
+ int nodelay = 1;
+ if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
+ reinterpret_cast<const char *>(&nodelay),
+ sizeof(nodelay)) != 0) {
+ PrintSocketError("setsockopt");
+ closesocket(sock);
+ return -1;
+ }
+
+ if (connect(sock, reinterpret_cast<const sockaddr *>(&addr), addr_len) != 0) {
+ PrintSocketError("connect");
+ closesocket(sock);
+ return -1;
+ }
+
+ return sock;
}
class SocketCloser {
@@ -775,7 +787,7 @@
#endif
}
- int sock = Connect(config->port);
+ int sock = Connect(config);
if (sock == -1) {
return false;
}
diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc
index 6516547..b6d8280 100644
--- a/ssl/test/handshake_util.cc
+++ b/ssl/test/handshake_util.cc
@@ -398,9 +398,9 @@
if (is_resume) {
args.push_back(kResumeFlag);
}
- // config->argv omits argv[0].
- for (int j = 0; j < config->argc; ++j) {
- args.push_back(config->argv[j]);
+ // config->handshaker_args omits argv[0].
+ for (const char *arg : config->handshaker_args) {
+ args.push_back(arg);
}
args.push_back(nullptr);
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index da0795b..e7ee194 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1260,25 +1260,32 @@
func newShimProcess(shimPath string, flags []string, env []string) (*shimProcess, error) {
shim := new(shimProcess)
var err error
+ useIPv6 := true
shim.listener, err = net.ListenTCP("tcp", &net.TCPAddr{IP: net.IPv6loopback})
if err != nil {
+ useIPv6 = false
shim.listener, err = net.ListenTCP("tcp4", &net.TCPAddr{IP: net.IP{127, 0, 0, 1}})
}
if err != nil {
return nil, err
}
- flags = append([]string{"-port", strconv.Itoa(shim.listener.Addr().(*net.TCPAddr).Port)}, flags...)
+ cmdFlags := []string{"-port", strconv.Itoa(shim.listener.Addr().(*net.TCPAddr).Port)}
+ if useIPv6 {
+ cmdFlags = append(cmdFlags, "-ipv6")
+ }
+ cmdFlags = append(cmdFlags, flags...)
+
if *useValgrind {
- shim.cmd = valgrindOf(false, shimPath, flags...)
+ shim.cmd = valgrindOf(false, shimPath, cmdFlags...)
} else if *useGDB {
- shim.cmd = gdbOf(shimPath, flags...)
+ shim.cmd = gdbOf(shimPath, cmdFlags...)
} else if *useLLDB {
- shim.cmd = lldbOf(shimPath, flags...)
+ shim.cmd = lldbOf(shimPath, cmdFlags...)
} else if *useRR {
- shim.cmd = rrOf(shimPath, flags...)
+ shim.cmd = rrOf(shimPath, cmdFlags...)
} else {
- shim.cmd = exec.Command(shimPath, flags...)
+ shim.cmd = exec.Command(shimPath, cmdFlags...)
}
shim.cmd.Stdin = os.Stdin
shim.cmd.Stdout = &shim.stdout
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 485a560..980bef1 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -46,12 +46,18 @@
struct Flag {
const char *name;
bool has_param;
+ // skip_handshaker, if true, causes this flag to be skipped when
+ // forwarding flags to the handshaker. This should be used with flags
+ // that only impact connecting to the runner.
+ bool skip_handshaker;
// If |has_param| is false, |param| will be nullptr.
std::function<bool(TestConfig *config, const char *param)> set_param;
};
-Flag BoolFlag(const char *name, bool TestConfig::*field) {
- return Flag{name, false, [=](TestConfig *config, const char *) -> bool {
+Flag BoolFlag(const char *name, bool TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, false, skip_handshaker,
+ [=](TestConfig *config, const char *) -> bool {
config->*field = true;
return true;
}};
@@ -91,15 +97,19 @@
}
template <typename T>
-Flag IntFlag(const char *name, T TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+Flag IntFlag(const char *name, T TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
return StringToInt(&(config->*field), param);
}};
}
template <typename T>
-Flag IntVectorFlag(const char *name, std::vector<T> TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+Flag IntVectorFlag(const char *name, std::vector<T> TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
T value;
if (!StringToInt(&value, param)) {
return false;
@@ -109,8 +119,10 @@
}};
}
-Flag StringFlag(const char *name, std::string TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+Flag StringFlag(const char *name, std::string TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
config->*field = param;
return true;
}};
@@ -119,8 +131,10 @@
// TODO(davidben): When we can depend on C++17 or Abseil, switch this to
// std::optional or absl::optional.
Flag OptionalStringFlag(const char *name,
- std::unique_ptr<std::string> TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+ std::unique_ptr<std::string> TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
(config->*field).reset(new std::string(param));
return true;
}};
@@ -143,15 +157,19 @@
return true;
}
-Flag Base64Flag(const char *name, std::string TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+Flag Base64Flag(const char *name, std::string TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
return DecodeBase64(&(config->*field), param);
}};
}
Flag Base64VectorFlag(const char *name,
- std::vector<std::string> TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+ std::vector<std::string> TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
std::string value;
if (!DecodeBase64(&value, param)) {
return false;
@@ -163,8 +181,10 @@
Flag StringPairVectorFlag(
const char *name,
- std::vector<std::pair<std::string, std::string>> TestConfig::*field) {
- return Flag{name, true, [=](TestConfig *config, const char *param) -> bool {
+ std::vector<std::pair<std::string, std::string>> TestConfig::*field,
+ bool skip_handshaker = false) {
+ return Flag{name, true, skip_handshaker,
+ [=](TestConfig *config, const char *param) -> bool {
const char *comma = strchr(param, ',');
if (!comma) {
return false;
@@ -178,7 +198,8 @@
std::vector<Flag> SortedFlags() {
std::vector<Flag> flags = {
- IntFlag("-port", &TestConfig::port),
+ IntFlag("-port", &TestConfig::port, /*skip_handshaker=*/true),
+ BoolFlag("-ipv6", &TestConfig::ipv6, /*skip_handshaker=*/true),
BoolFlag("-server", &TestConfig::is_server),
BoolFlag("-dtls", &TestConfig::is_dtls),
BoolFlag("-quic", &TestConfig::is_quic),
@@ -428,11 +449,10 @@
TestConfig *out_initial,
TestConfig *out_resume,
TestConfig *out_retry) {
- out_initial->argc = out_resume->argc = out_retry->argc = argc;
- out_initial->argv = out_resume->argv = out_retry->argv = argv;
for (int i = 0; i < argc; i++) {
bool skip = false;
- const char *name = argv[i];
+ const char *arg = argv[i];
+ const char *name = arg;
// -on-shim and -on-handshaker prefixes enable flags only on the shim or
// handshaker.
@@ -473,6 +493,13 @@
param = argv[i];
}
+ if (!flag->skip_handshaker) {
+ out_initial->handshaker_args.push_back(arg);
+ if (flag->has_param) {
+ out_initial->handshaker_args.push_back(param);
+ }
+ }
+
if (!skip) {
if (out != nullptr) {
if (!flag->set_param(out, param)) {
@@ -491,6 +518,8 @@
}
}
+ out_resume->handshaker_args = out_initial->handshaker_args;
+ out_retry->handshaker_args = out_initial->handshaker_args;
return true;
}
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index e5c6057..4e84800 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -26,6 +26,7 @@
struct TestConfig {
int port = 0;
+ bool ipv6 = false;
bool is_server = false;
bool is_dtls = false;
bool is_quic = false;
@@ -197,8 +198,7 @@
bool fips_202205 = false;
bool wpa_202304 = false;
- int argc;
- char **argv;
+ std::vector<const char*> handshaker_args;
bssl::UniquePtr<SSL_CTX> SetupCtx(SSL_CTX *old_ctx) const;
