Support reading output from IptablesRestoreController.
Add the ability to IptablesRestoreController to return the output
of a command. This is useful to run commands that list chains or
return counters through the ip[6]tables-restore.
Also enable unsigned-integer-overflow sanitization the unit tests
because their behaviour should be representative of actual code.
Having address sanitization enabled would have saved a fair
amount of time debugging an on-device abort() that did not affect
the tests.
Test: new unit test passes
Bug: 32323979
Change-Id: I70726ebbade0cb792aba38787c57378df177f2d8
diff --git a/server/Android.mk b/server/Android.mk
index 258c9a6..0b7cfbb 100644
--- a/server/Android.mk
+++ b/server/Android.mk
@@ -133,6 +133,7 @@
###
include $(CLEAR_VARS)
LOCAL_MODULE := netd_unit_test
+LOCAL_SANITIZE := unsigned-integer-overflow
LOCAL_CFLAGS := -Wall -Werror -Wunused-parameter
# Bug: http://b/29823425 Disable -Wvarargs for Clang update to r271374
LOCAL_CFLAGS += -Wno-varargs
diff --git a/server/IptablesBaseTest.cpp b/server/IptablesBaseTest.cpp
index b52ff9b..faa7433 100644
--- a/server/IptablesBaseTest.cpp
+++ b/server/IptablesBaseTest.cpp
@@ -82,11 +82,23 @@
return popen(realCmd.c_str(), "r");
}
-int IptablesBaseTest::fakeExecIptablesRestore(IptablesTarget target, const std::string& commands) {
+int IptablesBaseTest::fakeExecIptablesRestoreWithOutput(IptablesTarget target,
+ const std::string& commands,
+ std::string *output) {
sRestoreCmds.push_back({ target, commands });
+ if (output != nullptr) {
+ *output = sIptablesRestoreOutput.size() ? sIptablesRestoreOutput.front().c_str() : "";
+ }
+ if (sIptablesRestoreOutput.size()) {
+ sIptablesRestoreOutput.pop_front();
+ }
return 0;
}
+int IptablesBaseTest::fakeExecIptablesRestore(IptablesTarget target, const std::string& commands) {
+ return fakeExecIptablesRestoreWithOutput(target, commands, nullptr);
+}
+
int IptablesBaseTest::expectIptablesCommand(IptablesTarget target, int pos,
const std::string& cmd) {
@@ -160,3 +172,4 @@
std::vector<std::string> IptablesBaseTest::sCmds = {};
IptablesBaseTest::ExpectedIptablesCommands IptablesBaseTest::sRestoreCmds = {};
std::deque<std::string> IptablesBaseTest::sPopenContents = {};
+std::deque<std::string> IptablesBaseTest::sIptablesRestoreOutput = {};
diff --git a/server/IptablesBaseTest.h b/server/IptablesBaseTest.h
index d175c5b..5843361 100644
--- a/server/IptablesBaseTest.h
+++ b/server/IptablesBaseTest.h
@@ -30,6 +30,8 @@
static int fake_android_fork_execvp(int argc, char* argv[], int *status, bool, bool);
static int fakeExecIptables(IptablesTarget target, ...);
static int fakeExecIptablesRestore(IptablesTarget target, const std::string& commands);
+ static int fakeExecIptablesRestoreWithOutput(IptablesTarget target, const std::string& commands,
+ std::string *output);
static FILE *fake_popen(const char *cmd, const char *type);
void expectIptablesCommands(const std::vector<std::string>& expectedCmds);
void expectIptablesCommands(const ExpectedIptablesCommands& expectedCmds);
@@ -41,5 +43,6 @@
static std::vector<std::string> sCmds;
static ExpectedIptablesCommands sRestoreCmds;
static std::deque<std::string> sPopenContents;
+ static std::deque<std::string> sIptablesRestoreOutput;
int expectIptablesCommand(IptablesTarget target, int pos, const std::string& cmd);
};
diff --git a/server/IptablesRestoreController.cpp b/server/IptablesRestoreController.cpp
index 45f9ab6..ba3279c 100644
--- a/server/IptablesRestoreController.cpp
+++ b/server/IptablesRestoreController.cpp
@@ -130,8 +130,8 @@
int stderr_pipe[2];
if (pipe2(stdin_pipe, 0) == -1 ||
- pipe2(stdout_pipe, 0) == -1 ||
- pipe2(stderr_pipe, 0) == -1) {
+ pipe2(stdout_pipe, O_NONBLOCK) == -1 ||
+ pipe2(stderr_pipe, O_NONBLOCK) == -1) {
PLOG(ERROR) << "pipe2() failed";
return nullptr;
@@ -199,7 +199,8 @@
// TODO: Maybe we should keep a rotating buffer of the last N commands
// so that they can be dumped on dumpsys.
int IptablesRestoreController::sendCommand(const IptablesProcessType type,
- const std::string& command) {
+ const std::string& command,
+ std::string *output) {
std::unique_ptr<IptablesProcess> *process =
(type == IPTABLES_PROCESS) ? &mIpRestore : &mIp6Restore;
@@ -247,7 +248,7 @@
return -1;
}
- if (!drainAndWaitForAck(*process)) {
+ if (!drainAndWaitForAck(*process, output)) {
// drainAndWaitForAck has already logged an error.
return -1;
}
@@ -291,11 +292,10 @@
static constexpr int POLL_TIMEOUT_MS = 100;
/* static */
-bool IptablesRestoreController::drainAndWaitForAck(
- const std::unique_ptr<IptablesProcess> &process) {
+bool IptablesRestoreController::drainAndWaitForAck(const std::unique_ptr<IptablesProcess> &process,
+ std::string *output) {
bool receivedAck = false;
int timeout = 0;
- std::string out;
while (!receivedAck && (timeout++ < MAX_RETRIES)) {
int numEvents = TEMP_FAILURE_RETRY(
poll(process->pollFds, ARRAY_SIZE(process->pollFds), POLL_TIMEOUT_MS));
@@ -311,37 +311,40 @@
continue;
}
- char buffer[256];
+ char buffer[PIPE_BUF];
for (size_t i = 0; i < ARRAY_SIZE(process->pollFds); ++i) {
const struct pollfd &pollfd = process->pollFds[i];
if (pollfd.revents & POLLIN) {
- // TODO: We read a maximum of 256 bytes for each call to poll.
- // We should change this so that we can read as much input as we
- // can from the descriptor without blocking.
- const ssize_t size = TEMP_FAILURE_RETRY(read(pollfd.fd, buffer, sizeof(buffer)));
+ ssize_t size;
+ do {
+ size = TEMP_FAILURE_RETRY(read(pollfd.fd, buffer, sizeof(buffer)));
- // This should never happen. Poll just told us that we have
- // something available.
- if (size == -1) {
- PLOG(ERROR) << "Unable to read from descriptor";
- return false;
- }
-
- if (i == IptablesProcess::STDOUT_IDX) {
- // i == STDOUT_IDX : look for the ping response. We use
- // a string buffer here because it's possible (but unlikely)
- // that only a subsection of the PING response is available
- // on the pipe when poll returns for the first time. We use
- // find instead of operator== to be robust in the case of
- // additional stdout logging.
- out.append(buffer, size);
- if (out.find(PING) != std::string::npos) {
- receivedAck = true;
+ if (size == -1) {
+ if (errno != EAGAIN) {
+ PLOG(ERROR) << "Unable to read from descriptor";
+ }
+ break;
}
- } else {
- // i == STDERR_IDX implies stderr, log.
- IptablesRestoreController::maybeLogStderr(process, buffer, size);
- }
+
+ if (i == IptablesProcess::STDOUT_IDX) {
+ // i == STDOUT_IDX: accumulate stdout into *output, and look
+ // for the ping response.
+ output->append(buffer, size);
+ size_t pos = output->find(PING);
+ if (pos != std::string::npos) {
+ if (output->size() > pos + PING_SIZE) {
+ size_t extra = output->size() - (pos + PING_SIZE);
+ ALOGW("%zd extra characters after iptables response: '%s...'",
+ extra, output->substr(pos + PING_SIZE, 128).c_str());
+ }
+ output->resize(pos);
+ receivedAck = true;
+ }
+ } else {
+ // i == STDERR_IDX implies stderr, log.
+ IptablesRestoreController::maybeLogStderr(process, buffer, size);
+ }
+ } while (size > 0);
}
if (pollfd.revents & POLLHUP) {
// The pipe was closed. This likely means the subprocess is exiting, since
@@ -362,15 +365,21 @@
return receivedAck;
}
-int IptablesRestoreController::execute(const IptablesTarget target, const std::string& command) {
+int IptablesRestoreController::execute(const IptablesTarget target, const std::string& command,
+ std::string *output) {
std::lock_guard<std::mutex> lock(mLock);
+ std::string buffer;
+ if (output == nullptr) {
+ output = &buffer;
+ }
+
int res = 0;
if (target == V4 || target == V4V6) {
- res |= sendCommand(IPTABLES_PROCESS, command);
+ res |= sendCommand(IPTABLES_PROCESS, command, output);
}
if (target == V6 || target == V4V6) {
- res |= sendCommand(IP6TABLES_PROCESS, command);
+ res |= sendCommand(IP6TABLES_PROCESS, command, output);
}
return res;
}
diff --git a/server/IptablesRestoreController.h b/server/IptablesRestoreController.h
index 4279fac..f7ae292 100644
--- a/server/IptablesRestoreController.h
+++ b/server/IptablesRestoreController.h
@@ -34,6 +34,9 @@
// Execute |commands| on the given |target|.
int execute(const IptablesTarget target, const std::string& commands);
+ // Execute |commands| on the given |target|, and populate |output| with stdout.
+ int execute(const IptablesTarget target, const std::string& commands, std::string *output);
+
enum IptablesProcessType {
IPTABLES_PROCESS,
IP6TABLES_PROCESS,
@@ -53,11 +56,12 @@
private:
static IptablesProcess* forkAndExec(const IptablesProcessType type);
- int sendCommand(const IptablesProcessType type, const std::string& command);
+ int sendCommand(const IptablesProcessType type, const std::string& command,
+ std::string *output);
static std::string fixCommandString(const std::string& command);
- bool drainAndWaitForAck(const std::unique_ptr<IptablesProcess> &process);
+ bool drainAndWaitForAck(const std::unique_ptr<IptablesProcess> &process, std::string *output);
static void maybeLogStderr(const std::unique_ptr<IptablesProcess> &process,
const char* buf, const ssize_t numBytes);
diff --git a/server/IptablesRestoreControllerTest.cpp b/server/IptablesRestoreControllerTest.cpp
index c302b52..813028d 100644
--- a/server/IptablesRestoreControllerTest.cpp
+++ b/server/IptablesRestoreControllerTest.cpp
@@ -23,10 +23,12 @@
#define LOG_TAG "IptablesRestoreControllerTest"
#include <cutils/log.h>
#include <android-base/stringprintf.h>
+#include <android-base/strings.h>
#include "IptablesRestoreController.h"
#include "NetdConstants.h"
+using android::base::Join;
using android::base::StringPrintf;
class IptablesRestoreControllerTest : public ::testing::Test {
@@ -63,13 +65,18 @@
};
TEST_F(IptablesRestoreControllerTest, TestBasicCommand) {
- EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n"));
+ std::string output;
+
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n", nullptr));
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"));
+ EXPECT_EQ(0, con.execute(IptablesTarget::V6, "#Test\n", nullptr));
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4, "#Test\n", nullptr));
+
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n", &output));
+ EXPECT_EQ("#Test\n#Test\n", output); // One for IPv4 and one for IPv6.
// 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.
@@ -78,18 +85,22 @@
}
TEST_F(IptablesRestoreControllerTest, TestRestartOnMalformedCommand) {
+ std::string buffer;
for (int i = 0; i < 50; i++) {
IptablesTarget target = (IptablesTarget) (i % 3);
- ASSERT_EQ(-1, con.execute(target, "malformed command\n")) <<
+ std::string *output = (i % 2) ? &buffer : nullptr;
+ ASSERT_EQ(-1, con.execute(target, "malformed command\n", output)) <<
"Malformed command did not fail at iteration " << i;
- ASSERT_EQ(0, con.execute(target, "#Test\n")) <<
+ ASSERT_EQ(0, con.execute(target, "#Test\n", output)) <<
"No-op command did not succeed at iteration " << i;
}
}
TEST_F(IptablesRestoreControllerTest, TestRestartOnProcessDeath) {
+ std::string output;
+
// Run a command to ensure that the processes are running.
- EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n"));
+ EXPECT_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n", &output));
pid_t pid4 = getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS);
pid_t pid6 = getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS);
@@ -103,7 +114,7 @@
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_EQ(0, con.execute(IptablesTarget::V4V6, "#Test\n", nullptr));
EXPECT_NE(pid4, getIpRestorePid(IptablesRestoreController::IPTABLES_PROCESS));
EXPECT_NE(pid6, getIpRestorePid(IptablesRestoreController::IP6TABLES_PROCESS));
@@ -111,3 +122,43 @@
expectNoIptablesRestoreProcess(pid4);
expectNoIptablesRestoreProcess(pid6);
}
+
+TEST_F(IptablesRestoreControllerTest, TestOutput) {
+ const char *chainName = StringPrintf("netd_unit_test_%u", arc4random_uniform(10000)).c_str();
+
+ // Create a chain to list.
+ std::vector<std::string> createCommands = {
+ "*filter",
+ StringPrintf(":%s -", chainName),
+ StringPrintf("-A %s -j RETURN", chainName),
+ "COMMIT",
+ ""
+ };
+ EXPECT_EQ(0, con.execute(V4V6, Join(createCommands, "\n"), nullptr));
+
+ // Expected contents of the chain.
+ std::vector<std::string> expectedLines = {
+ StringPrintf("Chain %s (0 references)", chainName),
+ "target prot opt source destination ",
+ "RETURN all -- 0.0.0.0/0 0.0.0.0/0 ",
+ StringPrintf("Chain %s (0 references)", chainName),
+ "target prot opt source destination ",
+ "RETURN all ::/0 ::/0 ",
+ ""
+ };
+ std::string expected = Join(expectedLines, "\n");
+
+ // List and delete the chain.
+ std::vector<std::string> listCommands = {
+ "*filter",
+ StringPrintf("-n -L %s", chainName), // List chain.
+ StringPrintf(":%s -", chainName), // Flush chain (otherwise we can't delete it).
+ StringPrintf("-X %s", chainName), // Delete chain.
+ "COMMIT",
+ ""
+ };
+
+ std::string output;
+ EXPECT_EQ(0, con.execute(V4V6, Join(listCommands, "\n"), &output));
+ EXPECT_EQ(expected, output);
+}
diff --git a/server/NetdConstants.cpp b/server/NetdConstants.cpp
index f8a0d36..ff3fc2c 100644
--- a/server/NetdConstants.cpp
+++ b/server/NetdConstants.cpp
@@ -119,8 +119,13 @@
return res;
}
+int execIptablesRestoreWithOutput(IptablesTarget target, const std::string& commands,
+ std::string *output) {
+ return android::net::gCtls->iptablesRestoreCtrl.execute(target, commands, output);
+}
+
int execIptablesRestore(IptablesTarget target, const std::string& commands) {
- return android::net::gCtls->iptablesRestoreCtrl.execute(target, commands);
+ return execIptablesRestoreWithOutput(target, commands, nullptr);
}
/*
diff --git a/server/NetdConstants.h b/server/NetdConstants.h
index f283ed0..668b9be 100644
--- a/server/NetdConstants.h
+++ b/server/NetdConstants.h
@@ -45,6 +45,8 @@
int execIptables(IptablesTarget target, ...);
int execIptablesSilently(IptablesTarget target, ...);
int execIptablesRestore(IptablesTarget target, const std::string& commands);
+int execIptablesRestoreWithOutput(IptablesTarget target, const std::string& commands,
+ std::string *output);
bool isIfaceName(const char *name);
int parsePrefix(const char *prefix, uint8_t *family, void *address, int size, uint8_t *prefixlen);