More robust handling of iptables-restore process termination
Bug: 32323979
Test: unit tests pass
Test: bullhead builds and boots
Change-Id: Ib3ea4221b1b2025a0a236f2607db29e1cd30ffa9
diff --git a/server/IptablesRestoreControllerTest.cpp b/server/IptablesRestoreControllerTest.cpp
index 3176500..c302b52 100644
--- a/server/IptablesRestoreControllerTest.cpp
+++ b/server/IptablesRestoreControllerTest.cpp
@@ -15,32 +15,99 @@
*/
#include <string>
+#include <fcntl.h>
+#include <signal.h>
#include <gtest/gtest.h>
+#define LOG_TAG "IptablesRestoreControllerTest"
+#include <cutils/log.h>
+#include <android-base/stringprintf.h>
+
#include "IptablesRestoreController.h"
#include "NetdConstants.h"
-class IptablesRestoreControllerHolder {
-public:
- IptablesRestoreControllerHolder() {
- IptablesRestoreController::installSignalHandler(&ctrl);
- }
+using android::base::StringPrintf;
- IptablesRestoreController ctrl;
+class IptablesRestoreControllerTest : public ::testing::Test {
+public:
+ IptablesRestoreController con;
+
+ pid_t getIpRestorePid(const IptablesRestoreController::IptablesProcessType type) {
+ return con.getIpRestorePid(type);
+ };
+
+ void expectNoIptablesRestoreProcess(pid_t pid) {
+ // We can't readlink /proc/PID/exe, because zombie processes don't have it.
+ // Parse /proc/PID/stat instead.
+ std::string statPath = StringPrintf("/proc/%d/stat", pid);
+ int fd = open(statPath.c_str(), O_RDONLY);
+ if (fd == -1) {
+ // ENOENT means the process is gone (expected).
+ ASSERT_EQ(errno, ENOENT)
+ << "Unexpected error opening " << statPath << ": " << strerror(errno);
+ return;
+ }
+
+ // If the PID exists, it's possible (though very unlikely) that the PID was reused. Check the
+ // binary name as well, to ensure the test isn't flaky.
+ char statBuf[1024];
+ ASSERT_NE(-1, read(fd, statBuf, sizeof(statBuf)))
+ << "Could not read from " << statPath << ": " << strerror(errno);
+ close(fd);
+
+ std::string statString(statBuf);
+ EXPECT_FALSE(statString.find("iptables-restor") || statString.find("ip6tables-resto"))
+ << "Previous iptables-restore pid " << pid << " still alive: " << statString;
+ }
};
-IptablesRestoreControllerHolder holder;
-
-TEST(IptablesRestoreControllerTest, TestBasicCommand) {
- IptablesRestoreController& con = holder.ctrl;
- EXPECT_EQ(0, con.execute(IptablesTarget::V4, "#Test\n"));
+TEST_F(IptablesRestoreControllerTest, TestBasicCommand) {
EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n"));
+
+ pid_t pid4 = getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS);
+ pid_t pid6 = getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS);
+
EXPECT_EQ(0, con.execute(IptablesTarget::V6, "#Test\n"));
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4, "#Test\n"));
+
+ // Check the PIDs are the same as they were before. If they're not, the child processes were
+ // restarted, which causes a 30-60ms delay.
+ EXPECT_EQ(pid4, getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS));
+ EXPECT_EQ(pid6, getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS));
}
-TEST(IptablesRestoreControllerTest, TestRestartOnMalformedCommand) {
- IptablesRestoreController& con = holder.ctrl;
- EXPECT_EQ(-1, con.execute(IptablesTarget::V4V6, "malformed command\n"));
+TEST_F(IptablesRestoreControllerTest, TestRestartOnMalformedCommand) {
+ for (int i = 0; i < 50; i++) {
+ IptablesTarget target = (IptablesTarget) (i % 3);
+ ASSERT_EQ(-1, con.execute(target, "malformed command\n")) <<
+ "Malformed command did not fail at iteration " << i;
+ ASSERT_EQ(0, con.execute(target, "#Test\n")) <<
+ "No-op command did not succeed at iteration " << i;
+ }
+}
+
+TEST_F(IptablesRestoreControllerTest, TestRestartOnProcessDeath) {
+ // Run a command to ensure that the processes are running.
EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n"));
+
+ pid_t pid4 = getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS);
+ pid_t pid6 = getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS);
+
+ ASSERT_EQ(0, kill(pid4, 0)) << "iptables-restore pid " << pid4 << " does not exist";
+ ASSERT_EQ(0, kill(pid6, 0)) << "ip6tables-restore pid " << pid6 << " does not exist";
+ ASSERT_EQ(0, kill(pid4, SIGTERM)) << "Failed to send SIGTERM to iptables-restore pid " << pid4;
+ ASSERT_EQ(0, kill(pid6, SIGTERM)) << "Failed to send SIGTERM to ip6tables-restore pid " << pid6;
+
+ // Wait 100ms for processes to terminate.
+ TEMP_FAILURE_RETRY(usleep(100 * 1000));
+
+ // Ensure that running a new command properly restarts the processes.
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n"));
+ EXPECT_NE(pid4, getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS));
+ EXPECT_NE(pid6, getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS));
+
+ // Check there are no zombies.
+ expectNoIptablesRestoreProcess(pid4);
+ expectNoIptablesRestoreProcess(pid6);
}