Refactor: Encapsulate permissions and interfaces into a Network class.

Currently, there's a lot of logic in NetworkController surrounding events such
as interface addition/removal, network creation/destruction and default network
change, because these events are interwined. For example, adding an interface
means also adding a corresponding default network rule if the interface is being
added to the current default network.

When we introduce VPNs into this mix, things will get hairy real quick for all
this logic in NetworkController.

In this refactor, we introduce an abstract base class Network which supports
adding and removing interfaces. The main concrete implementation of this is
PhysicalNetwork, which allows setting permissions and "default network" state.

Since we've moved network permissions into the above class, and user permissions
into NetworkController, PermissionsController is unused and has been removed.

Also fix a few bugs in RouteController:
+ Use uidEnd correctly.
+ Check for all error cases in inet_pton.
+ Check the return value of android_fork_execvp() correctly.
+ The "return cmd1() && cmd2()" pattern is wrong. Rewrite that code.

Also (non-functional changes):
+ Remove instantiations of RouteController. It has static methods only.
+ Reorder some blocks in CommandListener so that the most frequent commands are
  checked first.
+ Remove unused paramError() and clearNetworkPreference().
+ Change all return codes to int (negative errno) wherever applicable.
+ Add WARN_UNUSED_RESULT everywhere.
+ Cleanup some style in RouteController and NetworkController.
+ Use uid_t instead of unsigned for user IDs.
+ Add clearer log messages at the source of failures.
+ Add a check for when fwmark bits are set without corresponding mask bits.

Bug: 15409918

Change-Id: Ibba78b0850160f9f3d17d476f16331a6db0025d1
diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index 90a9399..d0e1461 100644
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -17,27 +17,20 @@
 #include "RouteController.h"
 
 #include "Fwmark.h"
-#include "NetdConstants.h"
+
+#define LOG_TAG "Netd"
+#include "log/log.h"
+#include "logwrap/logwrap.h"
 
 #include <arpa/inet.h>
-#include <errno.h>
 #include <linux/fib_rules.h>
-#include <linux/netlink.h>
-#include <linux/rtnetlink.h>
-#include <logwrap/logwrap.h>
 #include <map>
-#include <netinet/in.h>
 #include <net/if.h>
-#include <sys/socket.h>
-#include <sys/uio.h>
-#include <unistd.h>
-
-// Avoids "non-constant-expression cannot be narrowed from type 'unsigned int' to 'unsigned short'"
-// warnings when using RTA_LENGTH(x) inside static initializers (even when x is already uint16_t).
-#define U16_RTA_LENGTH(x) static_cast<uint16_t>(RTA_LENGTH((x)))
 
 namespace {
 
+// BEGIN CONSTANTS --------------------------------------------------------------------------------
+
 const uint32_t RULE_PRIORITY_PRIVILEGED_LEGACY     = 11000;
 const uint32_t RULE_PRIORITY_PER_NETWORK_EXPLICIT  = 13000;
 const uint32_t RULE_PRIORITY_PER_NETWORK_INTERFACE = 14000;
@@ -50,8 +43,6 @@
 const uint32_t RULE_PRIORITY_UNREACHABLE           = 21000;
 #endif
 
-const uid_t INVALID_UID = static_cast<uid_t>(-1);
-
 // TODO: These should be turned into per-UID tables once the kernel supports UID-based routing.
 const int ROUTE_TABLE_PRIVILEGED_LEGACY = RouteController::ROUTE_TABLE_OFFSET_FROM_INDEX - 901;
 const int ROUTE_TABLE_LEGACY            = RouteController::ROUTE_TABLE_OFFSET_FROM_INDEX - 902;
@@ -66,9 +57,39 @@
              "Android-specific FRA_UID_{START,END} values also assigned in Linux uapi. "
              "Check that these values match what the kernel does and then update this assertion.");
 
+const uid_t INVALID_UID = static_cast<uid_t>(-1);
+
 const uint16_t NETLINK_REQUEST_FLAGS = NLM_F_REQUEST | NLM_F_ACK;
 const uint16_t NETLINK_CREATE_REQUEST_FLAGS = NETLINK_REQUEST_FLAGS | NLM_F_CREATE | NLM_F_EXCL;
 
+const sockaddr_nl NETLINK_ADDRESS = {AF_NETLINK, 0, 0, 0};
+
+const uint8_t AF_FAMILIES[] = {AF_INET, AF_INET6};
+
+const char* const IP_VERSIONS[] = {"-4", "-6"};
+
+// Avoids "non-constant-expression cannot be narrowed from type 'unsigned int' to 'unsigned short'"
+// warnings when using RTA_LENGTH(x) inside static initializers (even when x is already uint16_t).
+constexpr uint16_t U16_RTA_LENGTH(uint16_t x) {
+    return RTA_LENGTH(x);
+}
+
+// These are practically const, but can't be declared so, because they are used to initialize
+// non-const pointers ("void* iov_base") in iovec arrays.
+rtattr FRATTR_PRIORITY  = { U16_RTA_LENGTH(sizeof(uint32_t)), FRA_PRIORITY };
+rtattr FRATTR_TABLE     = { U16_RTA_LENGTH(sizeof(uint32_t)), FRA_TABLE };
+rtattr FRATTR_FWMARK    = { U16_RTA_LENGTH(sizeof(uint32_t)), FRA_FWMARK };
+rtattr FRATTR_FWMASK    = { U16_RTA_LENGTH(sizeof(uint32_t)), FRA_FWMASK };
+rtattr FRATTR_UID_START = { U16_RTA_LENGTH(sizeof(uid_t)),    FRA_UID_START };
+rtattr FRATTR_UID_END   = { U16_RTA_LENGTH(sizeof(uid_t)),    FRA_UID_END };
+
+rtattr RTATTR_TABLE     = { U16_RTA_LENGTH(sizeof(uint32_t)), RTA_TABLE };
+rtattr RTATTR_OIF       = { U16_RTA_LENGTH(sizeof(uint32_t)), RTA_OIF };
+
+uint8_t PADDING_BUFFER[RTA_ALIGNTO] = {0, 0, 0, 0};
+
+// END CONSTANTS ----------------------------------------------------------------------------------
+
 std::map<std::string, uint32_t> interfaceToIndex;
 
 uint32_t getRouteTableForInterface(const char* interface) {
@@ -79,8 +100,9 @@
         // If the interface goes away if_nametoindex() will return 0 but we still need to know
         // the index so we can remove the rules and routes.
         std::map<std::string, uint32_t>::iterator it = interfaceToIndex.find(interface);
-        if (it != interfaceToIndex.end())
+        if (it != interfaceToIndex.end()) {
             index = it->second;
+        }
     }
     return index ? index + RouteController::ROUTE_TABLE_OFFSET_FROM_INDEX : 0;
 }
@@ -89,7 +111,7 @@
 // |iov| is an array of struct iovec that contains the netlink message payload.
 // The netlink header is generated by this function based on |action| and |flags|.
 // Returns -errno if there was an error or if the kernel reported an error.
-int sendNetlinkRequest(uint16_t action, uint16_t flags, iovec* iov, int iovlen) {
+WARN_UNUSED_RESULT int sendNetlinkRequest(uint16_t action, uint16_t flags, iovec* iov, int iovlen) {
     nlmsghdr nlmsg = {
         .nlmsg_type = action,
         .nlmsg_flags = flags,
@@ -106,18 +128,23 @@
         nlmsgerr err;
     } response;
 
-    sockaddr_nl kernel = {AF_NETLINK, 0, 0, 0};
     int sock = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
     if (sock != -1 &&
-            connect(sock, reinterpret_cast<sockaddr*>(&kernel), sizeof(kernel)) != -1 &&
+            connect(sock, reinterpret_cast<const sockaddr*>(&NETLINK_ADDRESS),
+                    sizeof(NETLINK_ADDRESS)) != -1 &&
             writev(sock, iov, iovlen) != -1 &&
             (ret = recv(sock, &response, sizeof(response), 0)) != -1) {
         if (ret == sizeof(response)) {
             ret = response.err.error;  // Netlink errors are negative errno.
+            if (ret) {
+                ALOGE("netlink response contains error (%s)", strerror(-ret));
+            }
         } else {
+            ALOGE("bad netlink response message size (%d != %u)", ret, sizeof(response));
             ret = -EBADMSG;
         }
     } else {
+        ALOGE("netlink socket/connect/writev/recv failed (%s)", strerror(errno));
         ret = -errno;
     }
 
@@ -137,24 +164,32 @@
 // + If |interface| is non-NULL, the rule matches the specified outgoing interface.
 //
 // Returns 0 on success or negative errno on failure.
-int modifyIpRule(uint16_t action, uint32_t priority, uint32_t table, uint32_t fwmark, uint32_t mask,
-                 const char* interface, uid_t uidStart, uid_t uidEnd) {
+WARN_UNUSED_RESULT int modifyIpRule(uint16_t action, uint32_t priority, uint32_t table,
+                                    uint32_t fwmark, uint32_t mask, const char* interface,
+                                    uid_t uidStart, uid_t uidEnd) {
+    // Ensure that if you set a bit in the fwmark, it's not being ignored by the mask.
+    if (fwmark & ~mask) {
+        ALOGE("mask 0x%x does not select all the bits set in fwmark 0x%x", mask, fwmark);
+        return -ERANGE;
+    }
+
     // The interface name must include exactly one terminating NULL and be properly padded, or older
     // kernels will refuse to delete rules.
-    uint8_t padding[RTA_ALIGNTO] = {0, 0, 0, 0};
     uint16_t paddingLength = 0;
     size_t interfaceLength = 0;
     char oifname[IFNAMSIZ];
     if (interface) {
         interfaceLength = strlcpy(oifname, interface, IFNAMSIZ) + 1;
         if (interfaceLength > IFNAMSIZ) {
+            ALOGE("interface name too long (%u > %u)", interfaceLength, IFNAMSIZ);
             return -ENAMETOOLONG;
         }
         paddingLength = RTA_SPACE(interfaceLength) - RTA_LENGTH(interfaceLength);
     }
 
     // Either both start and end UID must be specified, or neither.
-    if ((uidStart == INVALID_UID) != (uidStart == INVALID_UID)) {
+    if ((uidStart == INVALID_UID) != (uidEnd == INVALID_UID)) {
+        ALOGE("incompatible start and end UIDs (%u vs %u)", uidStart, uidEnd);
         return -EUSERS;
     }
     bool isUidRule = (uidStart != INVALID_UID);
@@ -164,40 +199,32 @@
         .action = static_cast<uint8_t>(table ? FR_ACT_TO_TBL : FR_ACT_UNREACHABLE),
     };
 
-    rtattr fraPriority  = { U16_RTA_LENGTH(sizeof(priority)),  FRA_PRIORITY };
-    rtattr fraTable     = { U16_RTA_LENGTH(sizeof(table)),     FRA_TABLE };
-    rtattr fraFwmark    = { U16_RTA_LENGTH(sizeof(fwmark)),    FRA_FWMARK };
-    rtattr fraFwmask    = { U16_RTA_LENGTH(sizeof(mask)),      FRA_FWMASK };
-    rtattr fraOifname   = { U16_RTA_LENGTH(interfaceLength),   FRA_OIFNAME };
-    rtattr fraUidStart  = { U16_RTA_LENGTH(sizeof(uidStart)),  FRA_UID_START };
-    rtattr fraUidEnd    = { U16_RTA_LENGTH(sizeof(uidEnd)),    FRA_UID_END };
+    rtattr fraOifname = { U16_RTA_LENGTH(interfaceLength), FRA_OIFNAME };
 
     iovec iov[] = {
-        { NULL,          0 },
-        { &rule,         sizeof(rule) },
-        { &fraPriority,  sizeof(fraPriority) },
-        { &priority,     sizeof(priority) },
-        { &fraTable,     table ? sizeof(fraTable) : 0 },
-        { &table,        table ? sizeof(table) : 0 },
-        { &fraFwmark,    mask ? sizeof(fraFwmark) : 0 },
-        { &fwmark,       mask ? sizeof(fwmark) : 0 },
-        { &fraFwmask,    mask ? sizeof(fraFwmask) : 0 },
-        { &mask,         mask ? sizeof(mask) : 0 },
-        { &fraUidStart,  isUidRule ? sizeof(fraUidStart) : 0 },
-        { &uidStart,     isUidRule ? sizeof(uidStart) : 0 },
-        { &fraUidEnd,    isUidRule ? sizeof(fraUidEnd) : 0 },
-        { &uidEnd,       isUidRule ? sizeof(uidEnd) : 0 },
-        { &fraOifname,   interface ? sizeof(fraOifname) : 0 },
-        { oifname,       interfaceLength },
-        { padding,       paddingLength },
+        { NULL,              0 },
+        { &rule,             sizeof(rule) },
+        { &FRATTR_PRIORITY,  sizeof(FRATTR_PRIORITY) },
+        { &priority,         sizeof(priority) },
+        { &FRATTR_TABLE,     table ? sizeof(FRATTR_TABLE) : 0 },
+        { &table,            table ? sizeof(table) : 0 },
+        { &FRATTR_FWMARK,    mask ? sizeof(FRATTR_FWMARK) : 0 },
+        { &fwmark,           mask ? sizeof(fwmark) : 0 },
+        { &FRATTR_FWMASK,    mask ? sizeof(FRATTR_FWMASK) : 0 },
+        { &mask,             mask ? sizeof(mask) : 0 },
+        { &FRATTR_UID_START, isUidRule ? sizeof(FRATTR_UID_START) : 0 },
+        { &uidStart,         isUidRule ? sizeof(uidStart) : 0 },
+        { &FRATTR_UID_END,   isUidRule ? sizeof(FRATTR_UID_END) : 0 },
+        { &uidEnd,           isUidRule ? sizeof(uidEnd) : 0 },
+        { &fraOifname,       interface ? sizeof(fraOifname) : 0 },
+        { oifname,           interfaceLength },
+        { PADDING_BUFFER,    paddingLength },
     };
 
     uint16_t flags = (action == RTM_NEWRULE) ? NETLINK_CREATE_REQUEST_FLAGS : NETLINK_REQUEST_FLAGS;
-    uint8_t family[] = {AF_INET, AF_INET6};
-    for (size_t i = 0; i < ARRAY_SIZE(family); ++i) {
-        rule.family = family[i];
-        int ret = sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov));
-        if (ret) {
+    for (size_t i = 0; i < ARRAY_SIZE(AF_FAMILIES); ++i) {
+        rule.family = AF_FAMILIES[i];
+        if (int ret = sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov))) {
             return ret;
         }
     }
@@ -205,30 +232,29 @@
     return 0;
 }
 
-int modifyIpRule(uint16_t action, uint32_t priority, uint32_t table,
-                 uint32_t fwmark, uint32_t mask, const char* interface) {
-    return modifyIpRule(action, priority, table, fwmark, mask, interface, INVALID_UID, INVALID_UID);
-}
-
 // Adds or deletes an IPv4 or IPv6 route.
 // Returns 0 on success or negative errno on failure.
-int modifyIpRoute(uint16_t action, uint32_t table, const char* interface, const char* destination,
-                  const char* nexthop) {
+WARN_UNUSED_RESULT int modifyIpRoute(uint16_t action, uint32_t table, const char* interface,
+                                     const char* destination, const char* nexthop) {
     // At least the destination must be non-null.
     if (!destination) {
+        ALOGE("null destination");
         return -EFAULT;
     }
 
     // Parse the prefix.
     uint8_t rawAddress[sizeof(in6_addr)];
-    uint8_t family, prefixLength;
+    uint8_t family;
+    uint8_t prefixLength;
     int rawLength = parsePrefix(destination, &family, rawAddress, sizeof(rawAddress),
                                 &prefixLength);
     if (rawLength < 0) {
+        ALOGE("parsePrefix failed for destination %s (%s)", destination, strerror(-rawLength));
         return rawLength;
     }
 
     if (static_cast<size_t>(rawLength) > sizeof(rawAddress)) {
+        ALOGE("impossible! address too long (%d vs %u)", rawLength, sizeof(rawAddress));
         return -ENOBUFS;  // Cannot happen; parsePrefix only supports IPv4 and IPv6.
     }
 
@@ -237,68 +263,68 @@
     if (interface) {
         ifindex = if_nametoindex(interface);
         if (!ifindex) {
+            ALOGE("cannot find interface %s", interface);
             return -ENODEV;
         }
     }
 
     // If a nexthop was specified, parse it as the same family as the prefix.
     uint8_t rawNexthop[sizeof(in6_addr)];
-    if (nexthop && !inet_pton(family, nexthop, rawNexthop)) {
+    if (nexthop && inet_pton(family, nexthop, rawNexthop) <= 0) {
+        ALOGE("inet_pton failed for nexthop %s", nexthop);
         return -EINVAL;
     }
 
     // Assemble a rtmsg and put it in an array of iovec structures.
-    rtmsg rtmsg = {
+    rtmsg route = {
         .rtm_protocol = RTPROT_STATIC,
         .rtm_type = RTN_UNICAST,
         .rtm_family = family,
         .rtm_dst_len = prefixLength,
     };
 
-    rtattr rtaTable   = { U16_RTA_LENGTH(sizeof(table)),    RTA_TABLE };
-    rtattr rtaOif     = { U16_RTA_LENGTH(sizeof(ifindex)),  RTA_OIF };
-    rtattr rtaDst     = { U16_RTA_LENGTH(rawLength),        RTA_DST };
-    rtattr rtaGateway = { U16_RTA_LENGTH(rawLength),        RTA_GATEWAY };
+    rtattr rtaDst     = { U16_RTA_LENGTH(rawLength), RTA_DST };
+    rtattr rtaGateway = { U16_RTA_LENGTH(rawLength), RTA_GATEWAY };
 
     iovec iov[] = {
-        { NULL,         0 },
-        { &rtmsg,       sizeof(rtmsg) },
-        { &rtaTable,    sizeof(rtaTable) },
-        { &table,       sizeof(table) },
-        { &rtaDst,      sizeof(rtaDst) },
-        { rawAddress,   static_cast<size_t>(rawLength) },
-        { &rtaOif,      interface ? sizeof(rtaOif) : 0 },
-        { &ifindex,     interface ? sizeof(ifindex) : 0 },
-        { &rtaGateway,  nexthop ? sizeof(rtaGateway) : 0 },
-        { rawNexthop,   nexthop ? static_cast<size_t>(rawLength) : 0 },
+        { NULL,          0 },
+        { &route,        sizeof(route) },
+        { &RTATTR_TABLE, sizeof(RTATTR_TABLE) },
+        { &table,        sizeof(table) },
+        { &rtaDst,       sizeof(rtaDst) },
+        { rawAddress,    static_cast<size_t>(rawLength) },
+        { &RTATTR_OIF,   interface ? sizeof(RTATTR_OIF) : 0 },
+        { &ifindex,      interface ? sizeof(ifindex) : 0 },
+        { &rtaGateway,   nexthop ? sizeof(rtaGateway) : 0 },
+        { rawNexthop,    nexthop ? static_cast<size_t>(rawLength) : 0 },
     };
 
-    uint16_t flags = (action == RTM_NEWROUTE) ? NETLINK_CREATE_REQUEST_FLAGS : NETLINK_REQUEST_FLAGS;
+    uint16_t flags = (action == RTM_NEWROUTE) ? NETLINK_CREATE_REQUEST_FLAGS :
+                                                NETLINK_REQUEST_FLAGS;
     return sendNetlinkRequest(action, flags, iov, ARRAY_SIZE(iov));
 }
 
-int modifyPerNetworkRules(unsigned netId, const char* interface, Permission permission, bool add,
-                          bool modifyIptables) {
+WARN_UNUSED_RESULT int modifyPerNetworkRules(unsigned netId, const char* interface,
+                                             Permission permission, bool add, bool modifyIptables) {
     uint32_t table = getRouteTableForInterface(interface);
     if (!table) {
+        ALOGE("cannot find interface %s", interface);
         return -ESRCH;
     }
 
     uint16_t action = add ? RTM_NEWRULE : RTM_DELRULE;
-    int ret;
 
     Fwmark fwmark;
-    fwmark.permission = permission;
-
     Fwmark mask;
-    mask.permission = permission;
 
     // A rule to route traffic based on a chosen outgoing interface.
     //
     // Supports apps that use SO_BINDTODEVICE or IP_PKTINFO options and the kernel that already
     // knows the outgoing interface (typically for link-local communications).
-    if ((ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_INTERFACE, table, fwmark.intValue,
-                            mask.intValue, interface)) != 0) {
+    fwmark.permission = permission;
+    mask.permission = permission;
+    if (int ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_INTERFACE, table, fwmark.intValue,
+                               mask.intValue, interface, INVALID_UID, INVALID_UID)) {
         return ret;
     }
 
@@ -309,8 +335,8 @@
     // network stay on that network even if the default network changes.
     fwmark.netId = netId;
     mask.netId = FWMARK_NET_ID_MASK;
-    if ((ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_NORMAL, table, fwmark.intValue,
-                            mask.intValue, NULL)) != 0) {
+    if (int ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_NORMAL, table, fwmark.intValue,
+                               mask.intValue, NULL, INVALID_UID, INVALID_UID)) {
         return ret;
     }
 
@@ -318,12 +344,13 @@
     //
     // Supports apps that use the multinetwork APIs to restrict their traffic to a network.
     //
-    // We don't really need to check the permission bits of the fwmark here, as they would've been
-    // checked at the time the netId was set into the fwmark, but we do so to be consistent.
+    // Even though we check permissions at the time we set a netId into the fwmark of a socket, we
+    // still need to check it again in the rules here, because a network's permissions may have been
+    // updated via modifyNetworkPermission().
     fwmark.explicitlySelected = true;
     mask.explicitlySelected = true;
-    if ((ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_EXPLICIT, table, fwmark.intValue,
-                            mask.intValue, NULL)) != 0) {
+    if (int ret = modifyIpRule(action, RULE_PRIORITY_PER_NETWORK_EXPLICIT, table, fwmark.intValue,
+                               mask.intValue, NULL, INVALID_UID, INVALID_UID)) {
         return ret;
     }
 
@@ -335,11 +362,11 @@
     // + Mark sockets that accept connections from this interface so that the connection stays on
     //   the same interface.
     if (modifyIptables) {
-        const char* iptablesAction = add ? "-A" : "-D";
         char markString[UINT32_HEX_STRLEN];
         snprintf(markString, sizeof(markString), "0x%x", netId);
-        if (execIptables(V4V6, "-t", "mangle", iptablesAction, "INPUT", "-i", interface,
+        if (execIptables(V4V6, "-t", "mangle", add ? "-A" : "-D", "INPUT", "-i", interface,
                          "-j", "MARK", "--set-mark", markString, NULL)) {
+            ALOGE("failed to change iptables rule that sets incoming packet mark");
             return -EREMOTEIO;
         }
     }
@@ -347,29 +374,33 @@
     return 0;
 }
 
-int modifyDefaultNetworkRules(const char* interface, Permission permission, uint16_t action) {
+WARN_UNUSED_RESULT int modifyDefaultNetworkRules(const char* interface, Permission permission,
+                                                 uint16_t action) {
     uint32_t table = getRouteTableForInterface(interface);
     if (!table) {
+        ALOGE("cannot find interface %s", interface);
         return -ESRCH;
     }
 
     Fwmark fwmark;
-    fwmark.netId = 0;
-    fwmark.permission = permission;
-
     Fwmark mask;
+
+    fwmark.netId = 0;
     mask.netId = FWMARK_NET_ID_MASK;
+
+    fwmark.permission = permission;
     mask.permission = permission;
 
     return modifyIpRule(action, RULE_PRIORITY_DEFAULT_NETWORK, table, fwmark.intValue,
-                        mask.intValue, NULL);
+                        mask.intValue, NULL, INVALID_UID, INVALID_UID);
 }
 
-// Adds or removes an IPv4 or IPv6 route to the specified table and, if it's directly-connected
+// Adds or removes an IPv4 or IPv6 route to the specified table and, if it's a directly-connected
 // route, to the main table as well.
 // Returns 0 on success or negative errno on failure.
-int modifyRoute(const char* interface, const char* destination, const char* nexthop,
-                uint16_t action, RouteController::TableType tableType, unsigned /* uid */) {
+WARN_UNUSED_RESULT int modifyRoute(const char* interface, const char* destination,
+                                   const char* nexthop, uint16_t action,
+                                   RouteController::TableType tableType, uid_t /*uid*/) {
     uint32_t table = 0;
     switch (tableType) {
         case RouteController::INTERFACE: {
@@ -388,6 +419,7 @@
         }
     }
     if (!table) {
+        ALOGE("cannot find table for interface %s and tableType %d", interface, tableType);
         return -ESRCH;
     }
 
@@ -407,7 +439,7 @@
         // A failure with action == ADD && errno == EEXIST means that the route already exists in
         // the main table, perhaps because the kernel added it automatically as part of adding the
         // IP address to the interface. Ignore this, but complain about everything else.
-        if (ret != 0 && !(action == RTM_NEWROUTE && ret == -EEXIST)) {
+        if (ret && !(action == RTM_NEWROUTE && ret == -EEXIST)) {
             return ret;
         }
     }
@@ -415,52 +447,58 @@
     return 0;
 }
 
-bool flushRoutes(const char* interface) {
+// Returns 0 on success or negative errno on failure.
+WARN_UNUSED_RESULT int flushRoutes(const char* interface) {
     uint32_t table = getRouteTableForInterface(interface);
     if (!table) {
-        return false;
+        ALOGE("cannot find interface %s", interface);
+        return -ESRCH;
     }
     interfaceToIndex.erase(interface);
 
     char tableString[UINT32_STRLEN];
     snprintf(tableString, sizeof(tableString), "%u", table);
 
-    const char* version[] = {"-4", "-6"};
-    for (size_t i = 0; i < ARRAY_SIZE(version); ++i) {
+    for (size_t i = 0; i < ARRAY_SIZE(IP_VERSIONS); ++i) {
         const char* argv[] = {
             IP_PATH,
-            version[i],
+            IP_VERSIONS[i],
             "route"
             "flush",
             "table",
             tableString,
         };
-        int argc = ARRAY_SIZE(argv);
-
-        if (!android_fork_execvp(argc, const_cast<char**>(argv), NULL, false, false)) {
-            return false;
+        if (android_fork_execvp(ARRAY_SIZE(argv), const_cast<char**>(argv), NULL, false, false)) {
+            ALOGE("failed to flush routes");
+            return -EREMOTEIO;
         }
     }
 
-    return true;
+    return 0;
 }
 
 }  // namespace
 
-void RouteController::Init() {
+int RouteController::Init() {
+    Fwmark fwmark;
+    Fwmark mask;
+
     // Add a new rule to look up the 'main' table, with the same selectors as the "default network"
     // rule, but with a lower priority. Since the default network rule points to a table with a
     // default route, the rule we're adding will never be used for normal routing lookups. However,
     // the kernel may fall-through to it to find directly-connected routes when it validates that a
     // nexthop (in a route being added) is reachable.
-    Fwmark fwmark;
+    //
+    // TODO: This isn't true if the default network requires non-zero permissions. In that case, an
+    // app without those permissions may still be able to access directly-connected routes, since
+    // it won't match the default network rule. Fix this by only allowing the root UID (as a proxy
+    // for the kernel) to lookup this main table rule.
     fwmark.netId = 0;
-
-    Fwmark mask;
     mask.netId = FWMARK_NET_ID_MASK;
-
-    modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_MAIN, RT_TABLE_MAIN, fwmark.intValue, mask.intValue,
-                 NULL);
+    if (int ret = modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_MAIN, RT_TABLE_MAIN, fwmark.intValue,
+                               mask.intValue, NULL, INVALID_UID, INVALID_UID)) {
+        return ret;
+    }
 
     // Add rules to allow lookup of legacy routes.
     //
@@ -471,21 +509,28 @@
 
     fwmark.explicitlySelected = false;
     mask.explicitlySelected = true;
-
-    modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_LEGACY, ROUTE_TABLE_LEGACY, fwmark.intValue,
-                 mask.intValue, NULL);
+    if (int ret = modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_LEGACY, ROUTE_TABLE_LEGACY,
+                               fwmark.intValue, mask.intValue, NULL, INVALID_UID, INVALID_UID)) {
+        return ret;
+    }
 
     fwmark.permission = PERMISSION_CONNECTIVITY_INTERNAL;
     mask.permission = PERMISSION_CONNECTIVITY_INTERNAL;
 
-    modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_PRIVILEGED_LEGACY, ROUTE_TABLE_PRIVILEGED_LEGACY,
-                 fwmark.intValue, mask.intValue, NULL);
+    if (int ret = modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_PRIVILEGED_LEGACY,
+                               ROUTE_TABLE_PRIVILEGED_LEGACY, fwmark.intValue, mask.intValue, NULL,
+                               INVALID_UID, INVALID_UID)) {
+        return ret;
+    }
 
 // TODO: Uncomment once we are sure everything works.
 #if 0
     // Add a rule to preempt the pre-defined "from all lookup main" rule. This ensures that packets
     // that are already marked with a specific NetId don't fall-through to the main table.
-    modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_UNREACHABLE, 0, 0, 0, NULL);
+    return modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_UNREACHABLE, 0, 0, 0, NULL, INVALID_UID,
+                        INVALID_UID);
+#else
+    return 0;
 #endif
 }
 
@@ -496,15 +541,19 @@
 
 int RouteController::removeInterfaceFromNetwork(unsigned netId, const char* interface,
                                                 Permission permission) {
-    return modifyPerNetworkRules(netId, interface, permission, false, true) &&
-           flushRoutes(interface);
+    if (int ret = modifyPerNetworkRules(netId, interface, permission, false, true)) {
+        return ret;
+    }
+    return flushRoutes(interface);
 }
 
 int RouteController::modifyNetworkPermission(unsigned netId, const char* interface,
                                              Permission oldPermission, Permission newPermission) {
     // Add the new rules before deleting the old ones, to avoid race conditions.
-    return modifyPerNetworkRules(netId, interface, newPermission, true, false) &&
-           modifyPerNetworkRules(netId, interface, oldPermission, false, false);
+    if (int ret = modifyPerNetworkRules(netId, interface, newPermission, true, false)) {
+        return ret;
+    }
+    return modifyPerNetworkRules(netId, interface, oldPermission, false, false);
 }
 
 int RouteController::addToDefaultNetwork(const char* interface, Permission permission) {
@@ -515,12 +564,12 @@
     return modifyDefaultNetworkRules(interface, permission, RTM_DELRULE);
 }
 
-int RouteController::addRoute(const char* interface, const char* destination,
-                              const char* nexthop, TableType tableType, unsigned uid) {
+int RouteController::addRoute(const char* interface, const char* destination, const char* nexthop,
+                              TableType tableType, uid_t uid) {
     return modifyRoute(interface, destination, nexthop, RTM_NEWROUTE, tableType, uid);
 }
 
 int RouteController::removeRoute(const char* interface, const char* destination,
-                                 const char* nexthop, TableType tableType, unsigned uid) {
+                                 const char* nexthop, TableType tableType, uid_t uid) {
     return modifyRoute(interface, destination, nexthop, RTM_DELROUTE, tableType, uid);
 }