commit e3dc131e48120b91b3cf00572fbfeff325cd7a3f
author George Burgess IV <gbiv@google.com> Thu Feb 28 11:27:04 2019 -0800
committer George Burgess IV <gbiv@google.com> Wed Mar 06 18:28:40 2019 +0000
tree 4227f9044d3edf9ef735efbbe0ec7ef6c5e609fb
parent a219651ac91907cae7400a015d2d403f4e67fb5f

TetherController: Fix a memory and fd leak

Error paths (e.g. the one for setPosixSpawnAttrFlags/etc.) didn't
attempt to `free(args)`. Swapping to a vector neatly handles all of this
for us.

Caught by the static analyzer:
system/netd/server/TetherController.cpp:271:9: warning: Potential leak
of memory pointed to by 'args' [clang-analyzer-unix.Malloc]

Also caught by reviewers: we appear to leak a few FDs here in error
paths. This cleans those up, too.

Bug: None
Test: Ran the analyzer again. TreeHugger for functionality.
Change-Id: Ie53b3cdf4745aafa6f1e1284ccb7433ff345838e

server/TetherController.cpp

diff --git a/server/TetherController.cpp b/server/TetherController.cpp
index 4fc603a..bf1d177 100644
--- a/server/TetherController.cpp
+++ b/server/TetherController.cpp
@@ -36,8 +36,9 @@
 #include <vector>
 
 #define LOG_TAG "TetherController"
-#include <android-base/strings.h>
 #include <android-base/stringprintf.h>
+#include <android-base/strings.h>
+#include <android-base/unique_fd.h>
 #include <cutils/properties.h>
 #include <log/log.h>
 #include <netdutils/StatusOr.h>
@@ -55,7 +56,9 @@
 namespace net {
 
 using android::base::Join;
+using android::base::Pipe;
 using android::base::StringPrintf;
+using android::base::unique_fd;
 using android::netdutils::statusFromErrno;
 using android::netdutils::StatusOr;
 
@@ -212,9 +215,8 @@
 
     ALOGD("Starting tethering services");
 
-    int pipefd[2];
-
-    if (pipe2(pipefd, O_CLOEXEC) < 0) {
+    unique_fd pipeRead, pipeWrite;
+    if (!Pipe(&pipeRead, &pipeWrite, O_CLOEXEC)) {
         int res = errno;
         ALOGE("pipe2() failed (%s)", strerror(errno));
         return -res;
@@ -251,7 +253,7 @@
                                          dhcp_ranges[addrIndex + 1]));
     }
 
-    auto args = (char**)std::calloc(argVector.size() + 1, sizeof(char*));
+    std::vector<char*> args(argVector.size() + 1);
     for (unsigned i = 0; i < argVector.size(); i++) {
         args[i] = (char*)argVector[i].c_str();
     }
@@ -266,7 +268,7 @@
     // dup2 creates fd without CLOEXEC, dnsmasq will receive commands through the
     // duplicated fd.
     posix_spawn_file_actions_t fa;
-    int res = setPosixSpawnFileActionsAddDup2(&fa, pipefd[0], STDIN_FILENO);
+    int res = setPosixSpawnFileActionsAddDup2(&fa, pipeRead.get(), STDIN_FILENO);
     if (res) {
         ALOGE("posix_spawn set fa failed (%s)", strerror(res));
         return -res;
@@ -280,18 +282,15 @@
     }
 
     pid_t pid;
-    res = posix_spawn(&pid, args[0], &fa, &attr, args, nullptr);
-    close(pipefd[0]);
-    free(args);
+    res = posix_spawn(&pid, args[0], &fa, &attr, &args[0], nullptr);
     posix_spawnattr_destroy(&attr);
     posix_spawn_file_actions_destroy(&fa);
     if (res) {
         ALOGE("posix_spawn failed (%s)", strerror(res));
-        close(pipefd[1]);
         return -res;
     }
     mDaemonPid = pid;
-    mDaemonFd = pipefd[1];
+    mDaemonFd = pipeWrite.release();
     configureForTethering(true);
     applyDnsInterfaces();
     ALOGD("Tethering services running");