Test for races in IptablesRestoreController::Init.

(cherry picked from commit 2103b6ba8eb494e173b560c48319b4fba6821185)

Bug: 28362720
Test: angler builds, boots
Test: netd_{unit,integration}_test pass
Change-Id: I68d2becc608ca0f5160cc73cd244829f00e0d56f
Merged-In: Ifc7b7045f00f7803b31a22d96a04e208917af5a5

6 files changed, 68 insertions, 11 deletions

diff --git a/libnetdutils/Syscalls.cpp b/libnetdutils/Syscalls.cpp
index 00e2422a..97a0bc30 100644
--- a/libnetdutils/Syscalls.cpp
+++ b/libnetdutils/Syscalls.cpp
@@ -170,6 +170,14 @@ class RealSyscalls final : public Syscalls {
         return file;
     }
 
+    StatusOr<pid_t> fork() const override {
+        pid_t rv = ::fork();
+        if (rv == -1) {
+            return statusFromErrno(errno, "fork() failed");
+        }
+        return rv;
+    }
+
     StatusOr<int> vfprintf(FILE* file, const char* format, va_list ap) const override {
         auto rv = ::vfprintf(file, format, ap);
         if (rv == -1) {
diff --git a/libnetdutils/include/netdutils/MockSyscalls.h b/libnetdutils/include/netdutils/MockSyscalls.h
index f878acb2..5d141480 100644
--- a/libnetdutils/include/netdutils/MockSyscalls.h
+++ b/libnetdutils/include/netdutils/MockSyscalls.h
@@ -60,6 +60,7 @@ class MockSyscalls : public Syscalls {
     MOCK_CONST_METHOD3(vfprintf, StatusOr<int>(FILE* file, const char* format, va_list ap));
     MOCK_CONST_METHOD3(vfscanf, StatusOr<int>(FILE* file, const char* format, va_list ap));
     MOCK_CONST_METHOD1(fclose, Status(FILE* file));
+    MOCK_CONST_METHOD0(fork, StatusOr<pid_t>());
 };
 
 // For the lifetime of this mock, replace the contents of sSyscalls
diff --git a/libnetdutils/include/netdutils/Syscalls.h b/libnetdutils/include/netdutils/Syscalls.h
index eddd4ccb..f0c66cb7 100644
--- a/libnetdutils/include/netdutils/Syscalls.h
+++ b/libnetdutils/include/netdutils/Syscalls.h
@@ -20,6 +20,7 @@
 #include <memory>
 
 #include <poll.h>
+#include <unistd.h>
 #include <sys/eventfd.h>
 #include <sys/socket.h>
 #include <sys/types.h>
@@ -78,6 +79,8 @@ class Syscalls {
 
     virtual Status fclose(FILE* file) const = 0;
 
+    virtual StatusOr<pid_t> fork() const = 0;
+
     // va_args helpers
     // va_start doesn't work when the preceding argument is a reference
     // type so we're forced to use const char*.
diff --git a/server/IptablesRestoreController.cpp b/server/IptablesRestoreController.cpp
index e346b827..a90224a9 100644
--- a/server/IptablesRestoreController.cpp
+++ b/server/IptablesRestoreController.cpp
@@ -24,9 +24,13 @@
 #define LOG_TAG "IptablesRestoreController"
 #include <android-base/logging.h>
 #include <android-base/file.h>
+#include <netdutils/Syscalls.h>
 
 #include "Controllers.h"
 
+using android::netdutils::StatusOr;
+using android::netdutils::sSyscalls;
+
 constexpr char IPTABLES_RESTORE_PATH[] = "/system/bin/iptables-restore";
 constexpr char IP6TABLES_RESTORE_PATH[] = "/system/bin/ip6tables-restore";
 
@@ -113,6 +117,13 @@ public:
 };
 
 IptablesRestoreController::IptablesRestoreController() {
+    Init();
+}
+
+IptablesRestoreController::~IptablesRestoreController() {
+}
+
+void IptablesRestoreController::Init() {
     // Start the IPv4 and IPv6 processes in parallel, since each one takes 20-30ms.
     std::thread v4([this] () { mIpRestore.reset(forkAndExec(IPTABLES_PROCESS)); });
     std::thread v6([this] () { mIp6Restore.reset(forkAndExec(IP6TABLES_PROCESS)); });
@@ -120,9 +131,6 @@ IptablesRestoreController::IptablesRestoreController() {
     v6.join();
 }
 
-IptablesRestoreController::~IptablesRestoreController() {
-}
-
 /* static */
 IptablesProcess* IptablesRestoreController::forkAndExec(const IptablesProcessType type) {
     const char* const cmd = (type == IPTABLES_PROCESS) ?
@@ -142,8 +150,14 @@ IptablesProcess* IptablesRestoreController::forkAndExec(const IptablesProcessTyp
         return nullptr;
     }
 
-    pid_t child_pid = fork();
-    if (child_pid == 0) {
+    const auto& sys = sSyscalls.get();
+    StatusOr<pid_t> child_pid = sys.fork();
+    if (!isOk(child_pid)) {
+        ALOGE("fork() failed: %s", strerror(child_pid.status().code()));
+        return nullptr;
+    }
+
+    if (child_pid.value() == 0) {
         // The child process. Reads from stdin, writes to stderr and stdout.
 
         // stdin_pipe[1] : The write end of the stdin pipe.
@@ -183,11 +197,6 @@ IptablesProcess* IptablesRestoreController::forkAndExec(const IptablesProcessTyp
     }
 
     // The parent process. Writes to stdout and stderr and reads from stdin.
-    if (child_pid == -1) {
-        ALOGE("fork() failed: %s", strerror(errno));
-        return nullptr;
-    }
-
     // stdin_pipe[0] : The read end of the stdin pipe.
     // stdout_pipe[1] : The write end of the stdout pipe.
     // stderr_pipe[1] : The write end of the stderr pipe.
@@ -197,7 +206,7 @@ IptablesProcess* IptablesRestoreController::forkAndExec(const IptablesProcessTyp
         ALOGW("close() failed: %s", strerror(errno));
     }
 
-    return new IptablesProcess(child_pid, stdin_pipe[1], stdout_pipe[0], stderr_pipe[0]);
+    return new IptablesProcess(child_pid.value(), stdin_pipe[1], stdout_pipe[0], stderr_pipe[0]);
 }
 
 // TODO: Return -errno on failure instead of -1.
diff --git a/server/IptablesRestoreController.h b/server/IptablesRestoreController.h
index 6850d0de..b1c8dcd4 100644
--- a/server/IptablesRestoreController.h
+++ b/server/IptablesRestoreController.h
@@ -68,6 +68,8 @@ protected:
     // |POLL_TIMEOUT_MS * MAX_RETRIES|. Chosen so that the overall timeout is 1s.
     static int POLL_TIMEOUT_MS;
 
+    void Init();
+
 private:
     static IptablesProcess* forkAndExec(const IptablesProcessType type);
 
diff --git a/server/IptablesRestoreControllerTest.cpp b/server/IptablesRestoreControllerTest.cpp
index 20d46efe..017870f9 100644
--- a/server/IptablesRestoreControllerTest.cpp
+++ b/server/IptablesRestoreControllerTest.cpp
@@ -20,12 +20,14 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #define LOG_TAG "IptablesRestoreControllerTest"
 #include <cutils/log.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
+#include <netdutils/MockSyscalls.h>
 
 #include "IptablesRestoreController.h"
 #include "NetdConstants.h"
@@ -37,6 +39,9 @@
 
 using android::base::Join;
 using android::base::StringPrintf;
+using android::netdutils::ScopedMockSyscalls;
+using testing::Return;
+using testing::StrictMock;
 
 class IptablesRestoreControllerTest : public ::testing::Test {
 public:
@@ -60,6 +65,10 @@ public:
     deleteTestChain();
   }
 
+  void Init() {
+    con.Init();
+  }
+
   pid_t getIpRestorePid(const IptablesRestoreController::IptablesProcessType type) {
       return con.getIpRestorePid(type);
   };
@@ -260,3 +269,28 @@ TEST_F(IptablesRestoreControllerTest, TestUidRuleBenchmark) {
                 iterations, timeTaken, timeTaken / 2 / iterations);
     }
 }
+
+TEST_F(IptablesRestoreControllerTest, TestStartup) {
+  // Tests that IptablesRestoreController::Init never sets its processes to null pointers if
+  // fork() succeeds.
+  {
+    // Mock fork(), and check that initializing 100 times never results in a null pointer.
+    constexpr int NUM_ITERATIONS = 100;  // Takes 100-150ms on angler.
+    constexpr pid_t FAKE_PID = 2000000001;
+    StrictMock<ScopedMockSyscalls> sys;
+
+    EXPECT_CALL(sys, fork()).Times(NUM_ITERATIONS * 2).WillRepeatedly(Return(FAKE_PID));
+    for (int i = 0; i < NUM_ITERATIONS; i++) {
+      Init();
+      EXPECT_NE(0, getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS));
+      EXPECT_NE(0, getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS));
+    }
+  }
+
+  // The controller is now in an invalid state: the pipes are connected to working iptables
+  // processes, but the PIDs are set to FAKE_PID. Send a malformed command to ensure that the
+  // processes terminate and close the pipes, then send a valid command to have the controller
+  // re-initialize properly now that fork() is no longer mocked.
+  EXPECT_EQ(-1, con.execute(V4V6, "malformed command\n", nullptr));
+  EXPECT_EQ(0, con.execute(V4V6, "#Test\n", nullptr));
+}