commit 2cffd9485b888e4c140526989e0f10736dd3b602
author Maciej Żenczykowski <maze@google.com> Sun May 05 13:40:35 2019 -0700
committer Maciej Żenczykowski <maze@google.com> Sat May 11 17:27:39 2019 -0700
tree 83f56c2a2b328b10e7cff64f11b9411cecff1bff
parent a1699958e6ce6463a9b87deff99cee7e44dd23a5

TetherController - fix missing calls to posix_*_destroy() on error paths

This fixes a couple cases were errors can result in a missing calls to:
  posix_spawnattr_destroy(&attr);
  posix_spawn_file_actions_destroy(&fa);
which potentially results in a memory leak.

It's hard to accomplish this with helper functions, because if init fails,
you shouldn't call destroy, but if adddup2/setflags fails you need to...

Test: atest clatd_test libbpf_android_test libnetdbpf_test netd_integration_test netd_unit_test netdutils_test resolv_integration_test resolv_unit_test
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: If6e5d778e16e2ab69cd5f7d75824f320e1b0888c

server/TetherController.cpp

diff --git a/server/TetherController.cpp b/server/TetherController.cpp
index 0c5b6bf..9d56b3e 100644
--- a/server/TetherController.cpp
+++ b/server/TetherController.cpp
@@ -36,6 +36,7 @@
 #include <vector>
 
 #define LOG_TAG "TetherController"
+#include <android-base/scopeguard.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
 #include <android-base/unique_fd.h>
@@ -116,22 +117,6 @@
     return !strcmp(BP_TOOLS_MODE, bootmode);
 }
 
-int setPosixSpawnFileActionsAddDup2(posix_spawn_file_actions_t* fa, int fd, int new_fd) {
-    int res = posix_spawn_file_actions_init(fa);
-    if (res) {
-        return res;
-    }
-    return posix_spawn_file_actions_adddup2(fa, fd, new_fd);
-}
-
-int setPosixSpawnAttrFlags(posix_spawnattr_t* attr, short flags) {
-    int res = posix_spawnattr_init(attr);
-    if (res) {
-        return res;
-    }
-    return posix_spawnattr_setflags(attr, flags);
-}
-
 }  // namespace
 
 auto TetherController::iptablesRestoreFunction = execIptablesRestoreWithOutput;
@@ -267,23 +252,33 @@
     // dup2 creates fd without CLOEXEC, dnsmasq will receive commands through the
     // duplicated fd.
     posix_spawn_file_actions_t fa;
-    int res = setPosixSpawnFileActionsAddDup2(&fa, pipeRead.get(), STDIN_FILENO);
+    int res = posix_spawn_file_actions_init(&fa);
     if (res) {
-        ALOGE("posix_spawn set fa failed (%s)", strerror(res));
+        ALOGE("posix_spawn_file_actions_init failed (%s)", strerror(res));
+        return -res;
+    }
+    const android::base::ScopeGuard faGuard = [&] { posix_spawn_file_actions_destroy(&fa); };
+    res = posix_spawn_file_actions_adddup2(&fa, pipeRead.get(), STDIN_FILENO);
+    if (res) {
+        ALOGE("posix_spawn_file_actions_adddup2 failed (%s)", strerror(res));
         return -res;
     }
 
     posix_spawnattr_t attr;
-    res = setPosixSpawnAttrFlags(&attr, POSIX_SPAWN_USEVFORK);
+    res = posix_spawnattr_init(&attr);
     if (res) {
-        ALOGE("posix_spawn set attr flag failed (%s)", strerror(res));
+        ALOGE("posix_spawnattr_init failed (%s)", strerror(res));
+        return -res;
+    }
+    const android::base::ScopeGuard attrGuard = [&] { posix_spawnattr_destroy(&attr); };
+    res = posix_spawnattr_setflags(&attr, POSIX_SPAWN_USEVFORK);
+    if (res) {
+        ALOGE("posix_spawnattr_setflags failed (%s)", strerror(res));
         return -res;
     }
 
     pid_t pid;
     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));
         return -res;