From 9b3fd6721392b16dee00caeca9586070496471e1 Mon Sep 17 00:00:00 2001
From: Josh Gao <jmgao@google.com>
Date: Fri, 11 Dec 2015 10:52:55 -0800
Subject: [PATCH] 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
---
 adb/shell_service.cpp | 66 +++++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/adb/shell_service.cpp b/adb/shell_service.cpp
index 3fc70b026..0f893788b 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 @@ bool Subprocess::ForkAndExec() {
     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 @@ bool Subprocess::ForkAndExec() {
         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 @@ bool Subprocess::ForkAndExec() {
         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 @@ bool Subprocess::ForkAndExec() {
         return false;
     }
 
+    D("subprocess parent: completed");
     return true;
 }