Default implementation of read_command_output is based on popen right now since

we met some weird bug

src/butil/popen.cpp

@@ -15,6 +15,7 @@
 // Author: Zhangyi Chen (chenzhangyi01@baidu.com)
 // Date: 2017/11/04 17:37:43
 
+#include <gflags/gflags.h>
 #include "butil/build_config.h"
 #include "butil/logging.h"
 
@@ -31,7 +32,7 @@ uint64_t BAIDU_WEAK bthread_usleep(uint64_t microseconds);
 
 namespace butil {
 
-const int CHILD_STACK_SIZE = 64 * 1024;
+const int CHILD_STACK_SIZE = 256 * 1024;
 
 struct ChildArgs {
     const char* cmd;
@@ -48,7 +49,7 @@ int launch_child_process(void* args) {
     _exit(1);
 }
 
-int read_command_output(std::ostream& os, const char* cmd) {
+int read_command_output_through_clone(std::ostream& os, const char* cmd) {
     int pipe_fd[2];
     if (pipe(pipe_fd) != 0) {
         PLOG(ERROR) << "Fail to pipe";
@@ -71,7 +72,7 @@ int read_command_output(std::ostream& os, const char* cmd) {
     child_stack = child_stack_mem + CHILD_STACK_SIZE;  
                                // ^ Assume stack grows downward
     cpid = clone(launch_child_process, child_stack,
-                       __WCLONE | CLONE_VM | SIGCHLD, &args);
+                 __WCLONE | CLONE_VM | SIGCHLD | CLONE_UNTRACED, &args);
     if (cpid < 0) {
         PLOG(ERROR) << "Fail to clone child process";
         rc = -1;
@@ -97,7 +98,7 @@ int read_command_output(std::ostream& os, const char* cmd) {
     pipe_fd[0] = -1;
 
     for (;;) {
-        pid_t wpid = waitpid(cpid, &wstatus, WNOHANG);
+        pid_t wpid = waitpid(cpid, &wstatus, WNOHANG | __WALL);
         if (wpid > 0) {
             break;
         }
@@ -141,15 +142,15 @@ END:
     return rc;
 }
 
-}  // namespace butil 
+DEFINE_bool(run_command_through_clone, false,
+            "(Linux specific) Run command with clone syscall to "
+            "avoid the costly page table duplication");
 
-#else  // OS_LINUX
+#endif // OS_LINUX
 
 #include <stdio.h>
 
-namespace butil {
-
-int read_command_output(std::ostream& os, const char* cmd) {
+int read_command_output_through_popen(std::ostream& os, const char* cmd) {
     FILE* pipe = popen(cmd, "r");
     if (pipe == NULL) {
         return -1;
@@ -170,9 +171,31 @@ int read_command_output(std::ostream& os, const char* cmd) {
             // retry;
         }
     }
-    return pclose(pipe);
+
+    const int wstatus = pclose(pipe);
+
+    if (wstatus < 0) {
+        return wstatus;
+    }
+    if (WIFEXITED(wstatus)) {
+        return WEXITSTATUS(wstatus);
+    }
+    if (WIFSIGNALED(wstatus)) {
+        os << "Child process was killed by signal "
+           << WTERMSIG(wstatus);
+    }
+    errno = ECHILD;
+    return -1;
 }
 
-}  // namespace butil 
+int read_command_output(std::ostream& os, const char* cmd) {
+#if !defined(OS_LINUX)
+    return read_command_output_through_popen(os, cmd);
+#else
+    return FLAGS_run_command_through_clone
+        ? read_command_output_through_clone(os, cmd)
+        : read_command_output_through_popen(os, cmd);
+#endif
+}
 
-#endif  // OS_LINUX
+}  // namespace butil

test/popen_unittest.cpp

@@ -6,40 +6,121 @@
 #include "butil/popen.h"
 #include "butil/errno.h"
 #include "butil/strings/string_piece.h"
+#include "butil/build_config.h"
 #include <gtest/gtest.h>
 
+namespace butil {
+extern int read_command_output_through_clone(std::ostream&, const char*);
+extern int read_command_output_through_popen(std::ostream&, const char*);
+}
+
 namespace {
 
 class PopenTest : public testing::Test {
 };
 
-TEST(PopenTest, sanity) {
+TEST(PopenTest, posix_popen) {
+    std::ostringstream oss;
+    int rc = butil::read_command_output_through_popen(oss, "echo \"Hello World\"");
+    ASSERT_EQ(0, rc) << berror(errno);
+    ASSERT_EQ("Hello World\n", oss.str());
+
+    oss.str("");
+    rc = butil::read_command_output_through_popen(oss, "exit 1");
+    EXPECT_EQ(1, rc) << berror(errno);
+    ASSERT_TRUE(oss.str().empty()) << oss;
+    oss.str("");
+    rc = butil::read_command_output_through_popen(oss, "kill -9 $$");
+    ASSERT_EQ(-1, rc);
+    ASSERT_EQ(errno, ECHILD);
+    ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 9"));
+    oss.str("");
+    rc = butil::read_command_output_through_clone(oss, "kill -15 $$");
+    ASSERT_EQ(-1, rc);
+    ASSERT_EQ(errno, ECHILD);
+    ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 15"));
+
+    oss.str("");
+    ASSERT_EQ(0, butil::read_command_output_through_clone(oss, "for i in `seq 1 100000`; do echo -n '=' ; done"));
+    ASSERT_EQ(100000u, oss.str().length());
+    std::string expected;
+    expected.resize(100000, '=');
+    ASSERT_EQ(expected, oss.str());
+}
+
+#if defined(OS_LINUX)
+
+TEST(PopenTest, clone) {
     std::ostringstream oss;
-    int rc = butil::read_command_output(oss, "echo \"Hello World\"");
+    int rc = butil::read_command_output_through_clone(oss, "echo \"Hello World\"");
     ASSERT_EQ(0, rc) << berror(errno);
     ASSERT_EQ("Hello World\n", oss.str());
 
     oss.str("");
-    rc = butil::read_command_output(oss, "exit 1");
+    rc = butil::read_command_output_through_clone(oss, "exit 1");
     ASSERT_EQ(1, rc) << berror(errno);
-    ASSERT_TRUE(oss.str().empty()) << oss.str();
+    ASSERT_TRUE(oss.str().empty()) << oss;
     oss.str("");
-    rc = butil::read_command_output(oss, "kill -9 $$");
+    rc = butil::read_command_output_through_clone(oss, "kill -9 $$");
     ASSERT_EQ(-1, rc);
     ASSERT_EQ(errno, ECHILD);
     ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 9"));
     oss.str("");
-    rc = butil::read_command_output(oss, "kill -15 $$");
+    rc = butil::read_command_output_through_clone(oss, "kill -15 $$");
     ASSERT_EQ(-1, rc);
     ASSERT_EQ(errno, ECHILD);
     ASSERT_TRUE(butil::StringPiece(oss.str()).ends_with("was killed by signal 15"));
 
     oss.str("");
-    ASSERT_EQ(0, butil::read_command_output(oss, "for i in `seq 1 100000`; do echo -n '=' ; done"));
-    ASSERT_EQ(100000u, oss.str().length()) << oss.str();
+    ASSERT_EQ(0, butil::read_command_output_through_clone(oss, "for i in `seq 1 100000`; do echo -n '=' ; done"));
+    ASSERT_EQ(100000u, oss.str().length());
     std::string expected;
     expected.resize(100000, '=');
     ASSERT_EQ(expected, oss.str());
 }
 
+
+struct CounterArg {
+    volatile int64_t counter;
+    volatile bool stop;
+};
+
+static void* counter_thread(void* args) {
+    CounterArg* ca = (CounterArg*)args;
+    while (!ca->stop) {
+        ++ca->counter;
+    }
+    return NULL;
+}
+
+static int fork_thread(void* arg) {
+    usleep(100 * 1000);
+    _exit(0);
+}
+
+const int CHILD_STACK_SIZE = 64 * 1024;
+
+TEST(PopenTest, does_vfork_suspend_all_threads) {
+    pthread_t tid;
+    CounterArg ca = { 0 , false };
+    ASSERT_EQ(0, pthread_create(&tid, NULL, counter_thread, &ca));
+    usleep(100 * 1000);
+    char* child_stack_mem = (char*)malloc(CHILD_STACK_SIZE);
+    void* child_stack = child_stack_mem + CHILD_STACK_SIZE;  
+    const int64_t counter_before_fork = ca.counter;
+    pid_t cpid = clone(fork_thread, child_stack, CLONE_VFORK, NULL);
+    const int64_t counter_after_fork = ca.counter;
+    usleep(100 * 1000);
+    const int64_t counter_after_sleep = ca.counter;
+    int ws;
+    ca.stop = true;
+    pthread_join(tid, NULL);
+    std::cout << "bc=" << counter_before_fork << " ac=" << counter_after_fork
+              << " as=" << counter_after_sleep
+              << std::endl;
+    ASSERT_EQ(cpid, waitpid(cpid, &ws, __WALL));
+}
+
+#endif  // OS_LINUX
+
 }