Revert handshaker fd numbers and make StartProcess more flexible.
b571e77773 changed these fd numbers, but that interacts poorly with
cross-version tests. Instead, remove the assumptions StartProcess() was
making about the relationship between the two sets of fds.
Change-Id: If8fe62e4d20d22776e79e05e82cb5920cbb545ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47044
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc
index 0fdf47f..34cee47 100644
--- a/ssl/test/handshake_util.cc
+++ b/ssl/test/handshake_util.cc
@@ -27,6 +27,7 @@
#endif
#include <functional>
+#include <map>
#include <vector>
#include "async_bio.h"
@@ -379,9 +380,16 @@
posix_spawn_file_actions_t *actions_;
};
+// StartHandshaker starts the handshaker process and, on success, returns a
+// handle to the process in |*out|. It sets |*out_control| to a control pipe to
+// the process. |map_fds| maps from desired fd number in the child process to
+// the source fd in the calling process. |close_fds| is the list of additional
+// fds to close, which may overlap with |map_fds|. Other than stdin, stdout, and
+// stderr, the status of fds not listed in either set is undefined.
static bool StartHandshaker(ScopedProcess *out, ScopedFD *out_control,
const TestConfig *config, bool is_resume,
- posix_spawn_file_actions_t *actions) {
+ std::map<int, int> map_fds,
+ std::vector<int> close_fds) {
if (config->handshaker_path.empty()) {
fprintf(stderr, "no -handshaker-path specified\n");
return false;
@@ -411,15 +419,55 @@
return false;
}
ScopedFD scoped_control0(control[0]), scoped_control1(control[1]);
+ close_fds.push_back(control[0]);
+ map_fds[kFdControl] = control[1];
- assert(control[1] != kFdHandshakerToProxy);
- assert(control[1] != kFdProxyToHandshaker);
- if (posix_spawn_file_actions_addclose(actions, control[0]) != 0) {
+ posix_spawn_file_actions_t actions;
+ if (posix_spawn_file_actions_init(&actions) != 0) {
return false;
}
- if (control[1] != kFdControl &&
- posix_spawn_file_actions_adddup2(actions, control[1], kFdControl) != 0) {
- return false;
+ FileActionsDestroyer actions_destroyer(&actions);
+ for (int fd : close_fds) {
+ if (posix_spawn_file_actions_addclose(&actions, fd) != 0) {
+ return false;
+ }
+ }
+ if (!map_fds.empty()) {
+ int max_fd = STDERR_FILENO;
+ for (const auto &pair : map_fds) {
+ max_fd = std::max(max_fd, pair.first);
+ max_fd = std::max(max_fd, pair.second);
+ }
+ // |map_fds| may contain cycles, so make a copy of all the source fds.
+ // |posix_spawn| can only use |dup2|, not |dup|, so we assume |max_fd| is
+ // the last fd we care about inheriting. |temp_fds| maps from fd number in
+ // the parent process to a temporary fd number in the child process.
+ std::map<int, int> temp_fds;
+ int next_fd = max_fd + 1;
+ for (const auto &pair : map_fds) {
+ if (temp_fds.count(pair.second)) {
+ continue;
+ }
+ temp_fds[pair.second] = next_fd;
+ if (posix_spawn_file_actions_adddup2(&actions, pair.second, next_fd) !=
+ 0 ||
+ posix_spawn_file_actions_addclose(&actions, pair.second) != 0) {
+ return false;
+ }
+ next_fd++;
+ }
+ for (const auto &pair : map_fds) {
+ if (posix_spawn_file_actions_adddup2(&actions, temp_fds[pair.second],
+ pair.first) != 0) {
+ return false;
+ }
+ }
+ // Clean up temporary fds.
+ for (int fd = max_fd + 1; fd < next_fd; fd++) {
+ if (posix_spawn_file_actions_addclose(&actions, fd) != 0) {
+ return false;
+ }
+ }
}
fflush(stdout);
@@ -428,7 +476,7 @@
// MSan doesn't know that |posix_spawn| initializes its output, so initialize
// it to -1.
pid_t pid = -1;
- if (posix_spawn(&pid, args[0], actions, nullptr,
+ if (posix_spawn(&pid, args[0], &actions, nullptr,
const_cast<char *const *>(args.data()), environ) != 0) {
return false;
}
@@ -443,12 +491,6 @@
static bool RunHandshaker(BIO *bio, const TestConfig *config, bool is_resume,
Span<const uint8_t> input,
std::vector<uint8_t> *out) {
- posix_spawn_file_actions_t actions;
- if (posix_spawn_file_actions_init(&actions) != 0) {
- return false;
- }
- FileActionsDestroyer actions_destroyer(&actions);
-
int rfd[2], wfd[2];
// We use pipes, rather than some other mechanism, for their buffers. During
// the handshake, this process acts as a dumb proxy until receiving the
@@ -472,29 +514,12 @@
}
ScopedFD wfd0_closer(wfd[0]), wfd1_closer(wfd[1]);
- assert(kFdControl != rfd[0]);
- assert(kFdControl != wfd[1]);
- assert(kFdHandshakerToProxy != rfd[0]);
- assert(kFdProxyToHandshaker != wfd[1]);
-
- if (posix_spawn_file_actions_addclose(&actions, rfd[1]) != 0 ||
- posix_spawn_file_actions_addclose(&actions, wfd[0]) != 0) {
- return false;
- }
- if (rfd[0] != kFdProxyToHandshaker &&
- posix_spawn_file_actions_adddup2(&actions, rfd[0],
- kFdProxyToHandshaker) != 0) {
- return false;
- }
- if (wfd[1] != kFdHandshakerToProxy &&
- posix_spawn_file_actions_adddup2(&actions, wfd[1],
- kFdHandshakerToProxy) != 0) {
- return false;
- }
-
ScopedProcess handshaker;
ScopedFD control;
- if (!StartHandshaker(&handshaker, &control, config, is_resume, &actions)) {
+ if (!StartHandshaker(
+ &handshaker, &control, config, is_resume,
+ {{kFdProxyToHandshaker, rfd[0]}, {kFdHandshakerToProxy, wfd[1]}},
+ {rfd[1], wfd[0]})) {
return false;
}
@@ -534,14 +559,9 @@
static bool RequestHandshakeHint(const TestConfig *config, bool is_resume,
Span<const uint8_t> input, bool *out_has_hints,
std::vector<uint8_t> *out_hints) {
- posix_spawn_file_actions_t actions;
- if (posix_spawn_file_actions_init(&actions) != 0) {
- return false;
- }
- FileActionsDestroyer actions_destroyer(&actions);
ScopedProcess handshaker;
ScopedFD control;
- if (!StartHandshaker(&handshaker, &control, config, is_resume, &actions)) {
+ if (!StartHandshaker(&handshaker, &control, config, is_resume, {}, {})) {
return false;
}
diff --git a/ssl/test/handshake_util.h b/ssl/test/handshake_util.h
index 7fee20b..dda9206 100644
--- a/ssl/test/handshake_util.h
+++ b/ssl/test/handshake_util.h
@@ -59,9 +59,9 @@
constexpr char kControlMsgError = 'E'; // Handshaker hit an error
// The protocol between the proxy and handshaker uses these file descriptors.
-constexpr int kFdControl = 20; // Bi-directional dgram socket.
-constexpr int kFdProxyToHandshaker = 21; // Uni-directional pipe.
-constexpr int kFdHandshakerToProxy = 22; // Uni-directional pipe.
+constexpr int kFdControl = 3; // Bi-directional dgram socket.
+constexpr int kFdProxyToHandshaker = 4; // Uni-directional pipe.
+constexpr int kFdHandshakerToProxy = 5; // Uni-directional pipe.
#endif // HANDSHAKER_SUPPORTED
#endif // HEADER_TEST_HANDSHAKE
