adb: don't use setenv after forking.
Previously, for `adb shell`, we were using setenv after forking to set
up the child's environment. This would occasionally deadlock in the
child, which would cause the main thread to deadlock waiting for the
child to complete. This patch constructs the environment before forking
and passes it to execle, eliminating the deadlock.
Bug: http://b/25847115
Change-Id: I720d472770564b1449819ddaab945a89844244a8
diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index 3fc70b0..0f89378 100644
--- a/adb/shell_service.cpp
+++ b/adb/shell_service.cpp
@@ -88,6 +88,9 @@
#include <termios.h>
#include <memory>
+#include <string>
+#include <unordered_map>
+#include <vector>
#include <android-base/logging.h>
#include <android-base/stringprintf.h>
@@ -237,13 +240,50 @@
ScopedFd parent_error_sfd, child_error_sfd;
char pts_name[PATH_MAX];
- // Create a socketpair for the fork() child to report any errors back to
- // the parent. Since we use threads, logging directly from the child could
- // create a race condition.
+ // Create a socketpair for the fork() child to report any errors back to the parent. Since we
+ // use threads, logging directly from the child might deadlock due to locks held in another
+ // thread during the fork.
if (!CreateSocketpair(&parent_error_sfd, &child_error_sfd)) {
LOG(ERROR) << "failed to create pipe for subprocess error reporting";
}
+ // Construct the environment for the child before we fork.
+ passwd* pw = getpwuid(getuid());
+ std::unordered_map<std::string, std::string> env;
+
+ char** current = environ;
+ while (char* env_cstr = *current++) {
+ std::string env_string = env_cstr;
+ char* delimiter = strchr(env_string.c_str(), '=');
+ *delimiter++ = '\0';
+ env[env_string.c_str()] = delimiter;
+ }
+
+ if (pw != nullptr) {
+ // TODO: $HOSTNAME? Normally bash automatically sets that, but mksh doesn't.
+ env["HOME"] = pw->pw_dir;
+ env["LOGNAME"] = pw->pw_name;
+ env["USER"] = pw->pw_name;
+ env["SHELL"] = pw->pw_shell;
+ }
+
+ if (!terminal_type_.empty()) {
+ env["TERM"] = terminal_type_;
+ }
+
+ std::vector<std::string> joined_env;
+ for (auto it : env) {
+ const char* key = it.first.c_str();
+ const char* value = it.second.c_str();
+ joined_env.push_back(android::base::StringPrintf("%s=%s", key, value));
+ }
+
+ std::vector<const char*> cenv;
+ for (const std::string& str : joined_env) {
+ cenv.push_back(str.c_str());
+ }
+ cenv.push_back(nullptr);
+
if (type_ == SubprocessType::kPty) {
int fd;
pid_ = forkpty(&fd, pts_name, nullptr, nullptr);
@@ -286,26 +326,14 @@
parent_error_sfd.Reset();
close_on_exec(child_error_sfd.fd());
- // TODO: $HOSTNAME? Normally bash automatically sets that, but mksh doesn't.
- passwd* pw = getpwuid(getuid());
- if (pw != nullptr) {
- setenv("HOME", pw->pw_dir, 1);
- setenv("LOGNAME", pw->pw_name, 1);
- setenv("SHELL", pw->pw_shell, 1);
- setenv("USER", pw->pw_name, 1);
- }
- if (!terminal_type_.empty()) {
- setenv("TERM", terminal_type_.c_str(), 1);
- }
-
if (is_interactive()) {
- execl(_PATH_BSHELL, _PATH_BSHELL, "-", nullptr);
+ execle(_PATH_BSHELL, _PATH_BSHELL, "-", nullptr, cenv.data());
} else {
- execl(_PATH_BSHELL, _PATH_BSHELL, "-c", command_.c_str(), nullptr);
+ execle(_PATH_BSHELL, _PATH_BSHELL, "-c", command_.c_str(), nullptr, cenv.data());
}
WriteFdExactly(child_error_sfd.fd(), "exec '" _PATH_BSHELL "' failed");
child_error_sfd.Reset();
- exit(-1);
+ _Exit(1);
}
// Subprocess parent.
@@ -320,6 +348,7 @@
return false;
}
+ D("subprocess parent: exec completed");
if (protocol_ == SubprocessProtocol::kNone) {
// No protocol: all streams pass through the stdinout FD and hook
// directly into the local socket for raw data transfer.
@@ -357,6 +386,7 @@
return false;
}
+ D("subprocess parent: completed");
return true;
}
