From c20c85008d7ad1e8152c426bc4f0abf8196c7643 Mon Sep 17 00:00:00 2001
From: Luis Hector Chavez <lhchavez@google.com>
Date: Wed, 18 Jul 2018 17:43:28 -0700
Subject: [PATCH 1/3] adb: Preserve the original mount flags when remounting

This change preserves any additional flags mount flags (e.g. MS_NODEV)
that any of the mounts would have. This avoids unnecessarily opening up
permissions, and also allows kernels that have additional restrictions
about what mount flags can be used to be happy with the remounts.

Bug: 111618714
Test: `adb remount` works in Chrome OS
Test: `adb remount` works in sailfish_aosp

Change-Id: I20d9f2feaf3a47b93bfcdfb4164ee61546ec0b68
---
 adb/daemon/remount_service.cpp | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/adb/daemon/remount_service.cpp b/adb/daemon/remount_service.cpp
index 830b35d63..d6bba0d00 100644
--- a/adb/daemon/remount_service.cpp
+++ b/adb/daemon/remount_service.cpp
@@ -26,6 +26,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/mount.h>
+#include <sys/statvfs.h>
 #include <sys/vfs.h>
 #include <unistd.h>
 
@@ -145,6 +146,17 @@ static bool can_unshare_blocks(int fd, const char* dev) {
     return true;
 }
 
+static unsigned long get_mount_flags(int fd, const char* dir) {
+    struct statvfs st_vfs;
+    if (statvfs(dir, &st_vfs) == -1) {
+        // Even though we could not get the original mount flags, assume that
+        // the mount was originally read-only.
+        WriteFdFmt(fd, "statvfs of the %s mount failed: %s.\n", dir, strerror(errno));
+        return MS_RDONLY;
+    }
+    return st_vfs.f_flag;
+}
+
 static bool remount_partition(int fd, const char* dir) {
     if (!directory_exists(dir)) {
         return true;
@@ -163,14 +175,23 @@ static bool remount_partition(int fd, const char* dir) {
                    dir, dev.c_str(), strerror(errno));
         return false;
     }
-    if (mount(dev.c_str(), dir, "none", MS_REMOUNT | MS_BIND, nullptr) == -1) {
+
+    unsigned long remount_flags = get_mount_flags(fd, dir);
+    if ((remount_flags & MS_RDONLY) == 0) {
+        // Mount is already writable.
+        return true;
+    }
+    remount_flags &= ~MS_RDONLY;
+    remount_flags |= MS_REMOUNT;
+
+    if (mount(dev.c_str(), dir, "none", remount_flags | MS_BIND, nullptr) == -1) {
         // This is useful for cases where the superblock is already marked as
         // read-write, but the mount itself is read-only, such as containers
         // where the remount with just MS_REMOUNT is forbidden by the kernel.
         WriteFdFmt(fd, "remount of the %s mount failed: %s.\n", dir, strerror(errno));
         return false;
     }
-    if (mount(dev.c_str(), dir, "none", MS_REMOUNT, nullptr) == -1) {
+    if (mount(dev.c_str(), dir, "none", remount_flags, nullptr) == -1) {
         WriteFdFmt(fd, "remount of the %s superblock failed: %s\n", dir, strerror(errno));
         return false;
     }

From 095792c300d0563904224c5a81ad0709bcd234f6 Mon Sep 17 00:00:00 2001
From: Luis Hector Chavez <lhchavez@google.com>
Date: Wed, 18 Jul 2018 19:40:12 -0700
Subject: [PATCH 2/3] adb: Modernize the service creation

This change removes the void* argument passing and instead uses C++11
features to avoid having to handle memory manually.

Bug: None
Test: python ./system/core/adb/test_device.py

Change-Id: I6380245b2ca583591810e3e363c67c993a107621
---
 adb/adb.h                                     |   5 -
 adb/daemon/file_sync_service.cpp              |   5 +-
 adb/daemon/framebuffer_service.cpp            |  13 +-
 adb/daemon/framebuffer_service.h              |  24 ++
 adb/daemon/remount_service.cpp                |  45 ++--
 .../set_verity_enable_state_service.cpp       |  26 +-
 adb/daemon/set_verity_enable_state_service.h  |  24 ++
 adb/file_sync_service.h                       |   4 +-
 adb/remount_service.h                         |   4 +-
 adb/services.cpp                              | 235 ++++++++----------
 10 files changed, 204 insertions(+), 181 deletions(-)
 create mode 100644 adb/daemon/framebuffer_service.h
 create mode 100644 adb/daemon/set_verity_enable_state_service.h

diff --git a/adb/adb.h b/adb/adb.h
index c8841661d..7e9af9e3f 100644
--- a/adb/adb.h
+++ b/adb/adb.h
@@ -156,11 +156,6 @@ int create_jdwp_connection_fd(int jdwp_pid);
 
 int handle_forward_request(const char* service, atransport* transport, int reply_fd);
 
-#if !ADB_HOST
-void framebuffer_service(int fd, void* cookie);
-void set_verity_enabled_state_service(int fd, void* cookie);
-#endif
-
 /* packet allocator */
 apacket* get_apacket(void);
 void put_apacket(apacket* p);
diff --git a/adb/daemon/file_sync_service.cpp b/adb/daemon/file_sync_service.cpp
index f7be8f94b..0b713631e 100644
--- a/adb/daemon/file_sync_service.cpp
+++ b/adb/daemon/file_sync_service.cpp
@@ -525,12 +525,11 @@ static bool handle_sync_command(int fd, std::vector<char>& buffer) {
     return true;
 }
 
-void file_sync_service(int fd, void*) {
+void file_sync_service(android::base::unique_fd fd) {
     std::vector<char> buffer(SYNC_DATA_MAX);
 
-    while (handle_sync_command(fd, buffer)) {
+    while (handle_sync_command(fd.get(), buffer)) {
     }
 
     D("sync: done");
-    adb_close(fd);
 }
diff --git a/adb/daemon/framebuffer_service.cpp b/adb/daemon/framebuffer_service.cpp
index 20e03f9c7..9a620ab7f 100644
--- a/adb/daemon/framebuffer_service.cpp
+++ b/adb/daemon/framebuffer_service.cpp
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include "framebuffer_service.h"
+
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/fb.h>
@@ -55,8 +57,7 @@ struct fbinfo {
     unsigned int alpha_length;
 } __attribute__((packed));
 
-void framebuffer_service(int fd, void *cookie)
-{
+void framebuffer_service(android::base::unique_fd fd) {
     struct fbinfo fbinfo;
     unsigned int i, bsize;
     char buf[640];
@@ -65,7 +66,7 @@ void framebuffer_service(int fd, void *cookie)
     int fds[2];
     pid_t pid;
 
-    if (pipe2(fds, O_CLOEXEC) < 0) goto pipefail;
+    if (pipe2(fds, O_CLOEXEC) < 0) return;
 
     pid = fork();
     if (pid < 0) goto done;
@@ -168,7 +169,7 @@ void framebuffer_service(int fd, void *cookie)
     }
 
     /* write header */
-    if(!WriteFdExactly(fd, &fbinfo, sizeof(fbinfo))) goto done;
+    if (!WriteFdExactly(fd.get(), &fbinfo, sizeof(fbinfo))) goto done;
 
     /* write data */
     for(i = 0; i < fbinfo.size; i += bsize) {
@@ -176,13 +177,11 @@ void framebuffer_service(int fd, void *cookie)
       if (i + bsize > fbinfo.size)
         bsize = fbinfo.size - i;
       if(!ReadFdExactly(fd_screencap, buf, bsize)) goto done;
-      if(!WriteFdExactly(fd, buf, bsize)) goto done;
+      if (!WriteFdExactly(fd.get(), buf, bsize)) goto done;
     }
 
 done:
     adb_close(fds[0]);
 
     TEMP_FAILURE_RETRY(waitpid(pid, nullptr, 0));
-pipefail:
-    adb_close(fd);
 }
diff --git a/adb/daemon/framebuffer_service.h b/adb/daemon/framebuffer_service.h
new file mode 100644
index 000000000..d99c6fe14
--- /dev/null
+++ b/adb/daemon/framebuffer_service.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _DAEMON_FRAMEBUFFER_SERVICE_H_
+#define _DAEMON_FRAMEBUFFER_SERVICE_H_
+
+#include <android-base/unique_fd.h>
+
+void framebuffer_service(android::base::unique_fd fd);
+
+#endif  // _DAEMON_FRAMEBUFFER_SERVICE_H_
diff --git a/adb/daemon/remount_service.cpp b/adb/daemon/remount_service.cpp
index d6bba0d00..dfdc365e3 100644
--- a/adb/daemon/remount_service.cpp
+++ b/adb/daemon/remount_service.cpp
@@ -44,6 +44,7 @@
 #include "adb_unique_fd.h"
 #include "adb_utils.h"
 #include "fs_mgr.h"
+#include "set_verity_enable_state_service.h"
 
 // Returns the device used to mount a directory in /proc/mounts.
 static std::string find_proc_mount(const char* dir) {
@@ -218,14 +219,11 @@ static void reboot_for_remount(int fd, bool need_fsck) {
     android::base::SetProperty(ANDROID_RB_PROPERTY, reboot_cmd.c_str());
 }
 
-void remount_service(int fd, void* cookie) {
-    unique_fd close_fd(fd);
-
-    const char* cmd = reinterpret_cast<const char*>(cookie);
-    bool user_requested_reboot = cmd && !strcmp(cmd, "-R");
+void remount_service(android::base::unique_fd fd, const std::string& cmd) {
+    bool user_requested_reboot = cmd != "-R";
 
     if (getuid() != 0) {
-        WriteFdExactly(fd, "Not running as root. Try \"adb root\" first.\n");
+        WriteFdExactly(fd.get(), "Not running as root. Try \"adb root\" first.\n");
         return;
     }
 
@@ -246,7 +244,7 @@ void remount_service(int fd, void* cookie) {
         if (dev.empty() || !fs_has_shared_blocks(dev.c_str())) {
             continue;
         }
-        if (can_unshare_blocks(fd, dev.c_str())) {
+        if (can_unshare_blocks(fd.get(), dev.c_str())) {
             dedup.emplace(partition);
         }
     }
@@ -257,12 +255,12 @@ void remount_service(int fd, void* cookie) {
     if (user_requested_reboot) {
         if (!dedup.empty() || verity_enabled) {
             if (verity_enabled) {
-                set_verity_enabled_state_service(fd, nullptr);
+                set_verity_enabled_state_service(android::base::unique_fd(dup(fd.get())), false);
             }
-            reboot_for_remount(fd, !dedup.empty());
+            reboot_for_remount(fd.get(), !dedup.empty());
             return;
         }
-        WriteFdExactly(fd, "No reboot needed, skipping -R.\n");
+        WriteFdExactly(fd.get(), "No reboot needed, skipping -R.\n");
     }
 
     // If we need to disable-verity, but we also need to perform a recovery
@@ -271,17 +269,14 @@ void remount_service(int fd, void* cookie) {
     if (verity_enabled && dedup.empty()) {
         // Allow remount but warn of likely bad effects
         bool both = system_verified && vendor_verified;
-        WriteFdFmt(fd,
-                   "dm_verity is enabled on the %s%s%s partition%s.\n",
-                   system_verified ? "system" : "",
-                   both ? " and " : "",
-                   vendor_verified ? "vendor" : "",
-                   both ? "s" : "");
-        WriteFdExactly(fd,
+        WriteFdFmt(fd.get(), "dm_verity is enabled on the %s%s%s partition%s.\n",
+                   system_verified ? "system" : "", both ? " and " : "",
+                   vendor_verified ? "vendor" : "", both ? "s" : "");
+        WriteFdExactly(fd.get(),
                        "Use \"adb disable-verity\" to disable verity.\n"
                        "If you do not, remount may succeed, however, you will still "
                        "not be able to write to these volumes.\n");
-        WriteFdExactly(fd,
+        WriteFdExactly(fd.get(),
                        "Alternately, use \"adb remount -R\" to disable verity "
                        "and automatically reboot.\n");
     }
@@ -292,29 +287,29 @@ void remount_service(int fd, void* cookie) {
         if (dedup.count(partition)) {
             continue;
         }
-        success &= remount_partition(fd, partition.c_str());
+        success &= remount_partition(fd.get(), partition.c_str());
     }
 
     if (!dedup.empty()) {
-        WriteFdExactly(fd,
+        WriteFdExactly(fd.get(),
                        "The following partitions are deduplicated and cannot "
                        "yet be remounted:\n");
         for (const std::string& name : dedup) {
-            WriteFdFmt(fd, "  %s\n", name.c_str());
+            WriteFdFmt(fd.get(), "  %s\n", name.c_str());
         }
 
-        WriteFdExactly(fd,
+        WriteFdExactly(fd.get(),
                        "To reboot and un-deduplicate the listed partitions, "
                        "please retry with adb remount -R.\n");
         if (system_verified || vendor_verified) {
-            WriteFdExactly(fd, "Note: verity will be automatically disabled after reboot.\n");
+            WriteFdExactly(fd.get(), "Note: verity will be automatically disabled after reboot.\n");
         }
         return;
     }
 
     if (!success) {
-        WriteFdExactly(fd, "remount failed\n");
+        WriteFdExactly(fd.get(), "remount failed\n");
     } else {
-        WriteFdExactly(fd, "remount succeeded\n");
+        WriteFdExactly(fd.get(), "remount succeeded\n");
     }
 }
diff --git a/adb/daemon/set_verity_enable_state_service.cpp b/adb/daemon/set_verity_enable_state_service.cpp
index dbeee280d..0f804e933 100644
--- a/adb/daemon/set_verity_enable_state_service.cpp
+++ b/adb/daemon/set_verity_enable_state_service.cpp
@@ -16,6 +16,7 @@
 
 #define TRACE_TAG ADB
 
+#include "set_verity_enable_state_service.h"
 #include "sysdeps.h"
 
 #include <fcntl.h>
@@ -25,8 +26,8 @@
 #include <stdio.h>
 #include <sys/stat.h>
 
-#include "android-base/properties.h"
-#include "android-base/stringprintf.h"
+#include <android-base/properties.h>
+#include <android-base/stringprintf.h>
 #include <log/log_properties.h>
 
 #include "adb.h"
@@ -131,12 +132,9 @@ static bool set_avb_verity_enabled_state(int fd, AvbOps* ops, bool enable_verity
     return true;
 }
 
-void set_verity_enabled_state_service(int fd, void* cookie) {
-    unique_fd closer(fd);
+void set_verity_enabled_state_service(android::base::unique_fd fd, bool enable) {
     bool any_changed = false;
 
-    bool enable = (cookie != nullptr);
-
     // Figure out if we're using VB1.0 or VB2.0 (aka AVB) - by
     // contract, androidboot.vbmeta.digest is set by the bootloader
     // when using AVB).
@@ -147,12 +145,12 @@ void set_verity_enabled_state_service(int fd, void* cookie) {
     // VB1.0 dm-verity is only enabled on certain builds.
     if (!using_avb) {
         if (!kAllowDisableVerity) {
-            WriteFdFmt(fd, "%s-verity only works for userdebug builds\n",
+            WriteFdFmt(fd.get(), "%s-verity only works for userdebug builds\n",
                        enable ? "enable" : "disable");
         }
 
         if (!android::base::GetBoolProperty("ro.secure", false)) {
-            WriteFdFmt(fd, "verity not enabled - ENG build\n");
+            WriteFdFmt(fd.get(), "verity not enabled - ENG build\n");
             return;
         }
     }
@@ -160,7 +158,7 @@ void set_verity_enabled_state_service(int fd, void* cookie) {
     // Should never be possible to disable dm-verity on a USER build
     // regardless of using AVB or VB1.0.
     if (!__android_log_is_debuggable()) {
-        WriteFdFmt(fd, "verity cannot be disabled/enabled - USER build\n");
+        WriteFdFmt(fd.get(), "verity cannot be disabled/enabled - USER build\n");
         return;
     }
 
@@ -168,10 +166,10 @@ void set_verity_enabled_state_service(int fd, void* cookie) {
         // Yep, the system is using AVB.
         AvbOps* ops = avb_ops_user_new();
         if (ops == nullptr) {
-            WriteFdFmt(fd, "Error getting AVB ops\n");
+            WriteFdFmt(fd.get(), "Error getting AVB ops\n");
             return;
         }
-        if (set_avb_verity_enabled_state(fd, ops, enable)) {
+        if (set_avb_verity_enabled_state(fd.get(), ops, enable)) {
             any_changed = true;
         }
         avb_ops_user_free(ops);
@@ -181,14 +179,14 @@ void set_verity_enabled_state_service(int fd, void* cookie) {
         // read all fstab entries at once from all sources
         fstab = fs_mgr_read_fstab_default();
         if (!fstab) {
-            WriteFdFmt(fd, "Failed to read fstab\nMaybe run adb root?\n");
+            WriteFdFmt(fd.get(), "Failed to read fstab\nMaybe run adb root?\n");
             return;
         }
 
         // Loop through entries looking for ones that vold manages.
         for (int i = 0; i < fstab->num_entries; i++) {
             if (fs_mgr_is_verified(&fstab->recs[i])) {
-                if (set_verity_enabled_state(fd, fstab->recs[i].blk_device,
+                if (set_verity_enabled_state(fd.get(), fstab->recs[i].blk_device,
                                              fstab->recs[i].mount_point, enable)) {
                     any_changed = true;
                 }
@@ -197,6 +195,6 @@ void set_verity_enabled_state_service(int fd, void* cookie) {
     }
 
     if (any_changed) {
-        WriteFdFmt(fd, "Now reboot your device for settings to take effect\n");
+        WriteFdFmt(fd.get(), "Now reboot your device for settings to take effect\n");
     }
 }
diff --git a/adb/daemon/set_verity_enable_state_service.h b/adb/daemon/set_verity_enable_state_service.h
new file mode 100644
index 000000000..9f84f35ea
--- /dev/null
+++ b/adb/daemon/set_verity_enable_state_service.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _DAEMON_SET_VERITY_ENABLED_STATE_SERVICE_H_
+#define _DAEMON_SET_VERITY_ENABLED_STATE_SERVICE_H_
+
+#include <android-base/unique_fd.h>
+
+void set_verity_enabled_state_service(android::base::unique_fd fd, bool enable);
+
+#endif  // _DAEMON_SET_VERITY_ENABLED_STATE_SERVICE_H_
diff --git a/adb/file_sync_service.h b/adb/file_sync_service.h
index 6606efdfb..608b3ada3 100644
--- a/adb/file_sync_service.h
+++ b/adb/file_sync_service.h
@@ -20,6 +20,8 @@
 #include <string>
 #include <vector>
 
+#include <android-base/unique_fd.h>
+
 #define MKID(a,b,c,d) ((a) | ((b) << 8) | ((c) << 16) | ((d) << 24))
 
 #define ID_LSTAT_V1 MKID('S','T','A','T')
@@ -79,7 +81,7 @@ union syncmsg {
     } status;
 };
 
-void file_sync_service(int fd, void* cookie);
+void file_sync_service(android::base::unique_fd fd);
 bool do_sync_ls(const char* path);
 bool do_sync_push(const std::vector<const char*>& srcs, const char* dst, bool sync);
 bool do_sync_pull(const std::vector<const char*>& srcs, const char* dst,
diff --git a/adb/remount_service.h b/adb/remount_service.h
index 7bda1be20..45821eef2 100644
--- a/adb/remount_service.h
+++ b/adb/remount_service.h
@@ -19,7 +19,9 @@
 
 #include <string>
 
+#include <android-base/unique_fd.h>
+
 bool make_block_device_writable(const std::string&);
-void remount_service(int, void*);
+void remount_service(android::base::unique_fd, const std::string&);
 
 #endif
diff --git a/adb/services.cpp b/adb/services.cpp
index 1fa7ecc07..3d418cb92 100644
--- a/adb/services.cpp
+++ b/adb/services.cpp
@@ -37,6 +37,7 @@
 #include <android-base/parsenetaddress.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
+#include <android-base/unique_fd.h>
 #include <cutils/sockets.h>
 
 #if !ADB_HOST
@@ -49,6 +50,10 @@
 #include "adb.h"
 #include "adb_io.h"
 #include "adb_utils.h"
+#if !ADB_HOST
+#include "daemon/framebuffer_service.h"
+#include "daemon/set_verity_enable_state_service.h"
+#endif
 #include "file_sync_service.h"
 #include "remount_service.h"
 #include "services.h"
@@ -57,81 +62,67 @@
 #include "sysdeps.h"
 #include "transport.h"
 
-struct stinfo {
-    const char* service_name;
-    void (*func)(int fd, void *cookie);
-    int fd;
-    void *cookie;
-};
+namespace {
 
-static void service_bootstrap_func(void* x) {
-    stinfo* sti = reinterpret_cast<stinfo*>(x);
-    adb_thread_setname(android::base::StringPrintf("%s svc %d", sti->service_name, sti->fd));
-    sti->func(sti->fd, sti->cookie);
-    free(sti);
+void service_bootstrap_func(std::string service_name,
+                            std::function<void(android::base::unique_fd)> func,
+                            android::base::unique_fd fd) {
+    adb_thread_setname(android::base::StringPrintf("%s svc %d", service_name.c_str(), fd.get()));
+    func(std::move(fd));
 }
 
 #if !ADB_HOST
 
-void restart_root_service(int fd, void *cookie) {
+void restart_root_service(android::base::unique_fd fd) {
     if (getuid() == 0) {
-        WriteFdExactly(fd, "adbd is already running as root\n");
-        adb_close(fd);
-    } else {
-        if (!__android_log_is_debuggable()) {
-            WriteFdExactly(fd, "adbd cannot run as root in production builds\n");
-            adb_close(fd);
-            return;
-        }
-
-        android::base::SetProperty("service.adb.root", "1");
-        WriteFdExactly(fd, "restarting adbd as root\n");
-        adb_close(fd);
+        WriteFdExactly(fd.get(), "adbd is already running as root\n");
+        return;
     }
+    if (!__android_log_is_debuggable()) {
+        WriteFdExactly(fd.get(), "adbd cannot run as root in production builds\n");
+        return;
+    }
+
+    android::base::SetProperty("service.adb.root", "1");
+    WriteFdExactly(fd.get(), "restarting adbd as root\n");
 }
 
-void restart_unroot_service(int fd, void *cookie) {
+void restart_unroot_service(android::base::unique_fd fd) {
     if (getuid() != 0) {
-        WriteFdExactly(fd, "adbd not running as root\n");
-        adb_close(fd);
-    } else {
-        android::base::SetProperty("service.adb.root", "0");
-        WriteFdExactly(fd, "restarting adbd as non root\n");
-        adb_close(fd);
+        WriteFdExactly(fd.get(), "adbd not running as root\n");
+        return;
     }
+    android::base::SetProperty("service.adb.root", "0");
+    WriteFdExactly(fd.get(), "restarting adbd as non root\n");
 }
 
-void restart_tcp_service(int fd, void *cookie) {
-    int port = (int) (uintptr_t) cookie;
+void restart_tcp_service(android::base::unique_fd fd, int port) {
     if (port <= 0) {
-        WriteFdFmt(fd, "invalid port %d\n", port);
-        adb_close(fd);
+        WriteFdFmt(fd.get(), "invalid port %d\n", port);
         return;
     }
 
     android::base::SetProperty("service.adb.tcp.port", android::base::StringPrintf("%d", port));
-    WriteFdFmt(fd, "restarting in TCP mode port: %d\n", port);
-    adb_close(fd);
+    WriteFdFmt(fd.get(), "restarting in TCP mode port: %d\n", port);
 }
 
-void restart_usb_service(int fd, void *cookie) {
+void restart_usb_service(android::base::unique_fd fd) {
     android::base::SetProperty("service.adb.tcp.port", "0");
-    WriteFdExactly(fd, "restarting in USB mode\n");
-    adb_close(fd);
+    WriteFdExactly(fd.get(), "restarting in USB mode\n");
 }
 
-static bool reboot_service_impl(int fd, const char* arg) {
-    const char* reboot_arg = arg;
+bool reboot_service_impl(android::base::unique_fd fd, const std::string& arg) {
+    std::string reboot_arg = arg;
     bool auto_reboot = false;
 
-    if (strcmp(reboot_arg, "sideload-auto-reboot") == 0) {
+    if (reboot_arg == "sideload-auto-reboot") {
         auto_reboot = true;
         reboot_arg = "sideload";
     }
 
     // It reboots into sideload mode by setting "--sideload" or "--sideload_auto_reboot"
     // in the command file.
-    if (strcmp(reboot_arg, "sideload") == 0) {
+    if (reboot_arg == "sideload") {
         if (getuid() != 0) {
             WriteFdExactly(fd, "'adb root' is required for 'adb reboot sideload'.\n");
             return false;
@@ -151,8 +142,8 @@ static bool reboot_service_impl(int fd, const char* arg) {
 
     sync();
 
-    if (!reboot_arg || !reboot_arg[0]) reboot_arg = "adb";
-    std::string reboot_string = android::base::StringPrintf("reboot,%s", reboot_arg);
+    if (reboot_arg.empty()) reboot_arg = "adb";
+    std::string reboot_string = android::base::StringPrintf("reboot,%s", reboot_arg.c_str());
     if (!android::base::SetProperty(ANDROID_RB_PROPERTY, reboot_string)) {
         WriteFdFmt(fd, "reboot (%s) failed\n", reboot_string.c_str());
         return false;
@@ -161,23 +152,19 @@ static bool reboot_service_impl(int fd, const char* arg) {
     return true;
 }
 
-void reboot_service(int fd, void* arg) {
-    if (reboot_service_impl(fd, static_cast<const char*>(arg))) {
-        // Don't return early. Give the reboot command time to take effect
-        // to avoid messing up scripts which do "adb reboot && adb wait-for-device"
-        while (true) {
-            pause();
-        }
+void reboot_service(android::base::unique_fd fd, const std::string& arg) {
+    if (!reboot_service_impl(std::move(fd), arg)) {
+        return;
+    }
+    // Don't return early. Give the reboot command time to take effect
+    // to avoid messing up scripts which do "adb reboot && adb wait-for-device"
+    while (true) {
+        pause();
     }
-
-    free(arg);
-    adb_close(fd);
 }
 
-static void reconnect_service(int fd, void* arg) {
+void reconnect_service(android::base::unique_fd fd, atransport* t) {
     WriteFdExactly(fd, "done");
-    adb_close(fd);
-    atransport* t = static_cast<atransport*>(arg);
     kick_transport(t);
 }
 
@@ -197,7 +184,7 @@ int reverse_service(const char* command, atransport* transport) {
 
 // Shell service string can look like:
 //   shell[,arg1,arg2,...]:[command]
-static int ShellService(const std::string& args, const atransport* transport) {
+int ShellService(const std::string& args, const atransport* transport) {
     size_t delimiter_index = args.find(':');
     if (delimiter_index == std::string::npos) {
         LOG(ERROR) << "No ':' found in shell service arguments: " << args;
@@ -236,16 +223,17 @@ static int ShellService(const std::string& args, const atransport* transport) {
 
 #endif  // !ADB_HOST
 
-static int create_service_thread(const char* service_name, void (*func)(int, void*), void* cookie) {
+android::base::unique_fd create_service_thread(const char* service_name,
+                                               std::function<void(android::base::unique_fd)> func) {
     int s[2];
     if (adb_socketpair(s)) {
         printf("cannot create service socket pair\n");
-        return -1;
+        return android::base::unique_fd();
     }
     D("socketpair: (%d,%d)", s[0], s[1]);
 
 #if !ADB_HOST
-    if (func == &file_sync_service) {
+    if (strcmp(service_name, "sync") == 0) {
         // Set file sync service socket to maximum size
         int max_buf = LINUX_MAX_SOCKET_SIZE;
         adb_setsockopt(s[0], SOL_SOCKET, SO_SNDBUF, &max_buf, sizeof(max_buf));
@@ -253,21 +241,14 @@ static int create_service_thread(const char* service_name, void (*func)(int, voi
     }
 #endif // !ADB_HOST
 
-    stinfo* sti = reinterpret_cast<stinfo*>(malloc(sizeof(stinfo)));
-    if (sti == nullptr) {
-        fatal("cannot allocate stinfo");
-    }
-    sti->service_name = service_name;
-    sti->func = func;
-    sti->cookie = cookie;
-    sti->fd = s[1];
-
-    std::thread(service_bootstrap_func, sti).detach();
+    std::thread(service_bootstrap_func, service_name, func, android::base::unique_fd(s[1])).detach();
 
     D("service thread started, %d:%d",s[0], s[1]);
-    return s[0];
+    return android::base::unique_fd(s[0]);
 }
 
+}  // namespace
+
 int service_to_fd(const char* name, atransport* transport) {
     int ret = -1;
 
@@ -280,54 +261,60 @@ int service_to_fd(const char* name, atransport* transport) {
 #if !ADB_HOST
     } else if(!strncmp("dev:", name, 4)) {
         ret = unix_open(name + 4, O_RDWR | O_CLOEXEC);
-    } else if(!strncmp(name, "framebuffer:", 12)) {
-        ret = create_service_thread("fb", framebuffer_service, nullptr);
+    } else if (!strncmp(name, "framebuffer:", 12)) {
+        ret = create_service_thread("fb", framebuffer_service).release();
     } else if (!strncmp(name, "jdwp:", 5)) {
-        ret = create_jdwp_connection_fd(atoi(name+5));
-    } else if(!strncmp(name, "shell", 5)) {
+        ret = create_jdwp_connection_fd(atoi(name + 5));
+    } else if (!strncmp(name, "shell", 5)) {
         ret = ShellService(name + 5, transport);
-    } else if(!strncmp(name, "exec:", 5)) {
+    } else if (!strncmp(name, "exec:", 5)) {
         ret = StartSubprocess(name + 5, nullptr, SubprocessType::kRaw, SubprocessProtocol::kNone);
-    } else if(!strncmp(name, "sync:", 5)) {
-        ret = create_service_thread("sync", file_sync_service, nullptr);
-    } else if(!strncmp(name, "remount:", 8)) {
-        const char* options = name + strlen("remount:");
-        void* cookie = const_cast<void*>(reinterpret_cast<const void*>(options));
-        ret = create_service_thread("remount", remount_service, cookie);
-    } else if(!strncmp(name, "reboot:", 7)) {
-        void* arg = strdup(name + 7);
-        if (arg == NULL) return -1;
-        ret = create_service_thread("reboot", reboot_service, arg);
-        if (ret < 0) free(arg);
-    } else if(!strncmp(name, "root:", 5)) {
-        ret = create_service_thread("root", restart_root_service, nullptr);
-    } else if(!strncmp(name, "unroot:", 7)) {
-        ret = create_service_thread("unroot", restart_unroot_service, nullptr);
-    } else if(!strncmp(name, "backup:", 7)) {
-        ret = StartSubprocess(android::base::StringPrintf("/system/bin/bu backup %s",
-                                                          (name + 7)).c_str(),
-                              nullptr, SubprocessType::kRaw, SubprocessProtocol::kNone);
-    } else if(!strncmp(name, "restore:", 8)) {
+    } else if (!strncmp(name, "sync:", 5)) {
+        ret = create_service_thread("sync", file_sync_service).release();
+    } else if (!strncmp(name, "remount:", 8)) {
+        std::string options(name + strlen("remount:"));
+        ret = create_service_thread("remount",
+                                    std::bind(remount_service, std::placeholders::_1, options))
+                      .release();
+    } else if (!strncmp(name, "reboot:", 7)) {
+        std::string arg(name + strlen("reboot:"));
+        ret = create_service_thread("reboot", std::bind(reboot_service, std::placeholders::_1, arg))
+                      .release();
+    } else if (!strncmp(name, "root:", 5)) {
+        ret = create_service_thread("root", restart_root_service).release();
+    } else if (!strncmp(name, "unroot:", 7)) {
+        ret = create_service_thread("unroot", restart_unroot_service).release();
+    } else if (!strncmp(name, "backup:", 7)) {
+        ret = StartSubprocess(
+                android::base::StringPrintf("/system/bin/bu backup %s", (name + 7)).c_str(),
+                nullptr, SubprocessType::kRaw, SubprocessProtocol::kNone);
+    } else if (!strncmp(name, "restore:", 8)) {
         ret = StartSubprocess("/system/bin/bu restore", nullptr, SubprocessType::kRaw,
                               SubprocessProtocol::kNone);
-    } else if(!strncmp(name, "tcpip:", 6)) {
+    } else if (!strncmp(name, "tcpip:", 6)) {
         int port;
         if (sscanf(name + 6, "%d", &port) != 1) {
             return -1;
         }
-        ret = create_service_thread("tcp", restart_tcp_service, reinterpret_cast<void*>(port));
-    } else if(!strncmp(name, "usb:", 4)) {
-        ret = create_service_thread("usb", restart_usb_service, nullptr);
+        ret = create_service_thread("tcp",
+                                    std::bind(restart_tcp_service, std::placeholders::_1, port))
+                      .release();
+    } else if (!strncmp(name, "usb:", 4)) {
+        ret = create_service_thread("usb", restart_usb_service).release();
     } else if (!strncmp(name, "reverse:", 8)) {
         ret = reverse_service(name + 8, transport);
-    } else if(!strncmp(name, "disable-verity:", 15)) {
-        ret = create_service_thread("verity-on", set_verity_enabled_state_service,
-                                    reinterpret_cast<void*>(0));
-    } else if(!strncmp(name, "enable-verity:", 15)) {
-        ret = create_service_thread("verity-off", set_verity_enabled_state_service,
-                                    reinterpret_cast<void*>(1));
+    } else if (!strncmp(name, "disable-verity:", 15)) {
+        ret = create_service_thread("verity-on", std::bind(set_verity_enabled_state_service,
+                                                           std::placeholders::_1, false))
+                      .release();
+    } else if (!strncmp(name, "enable-verity:", 15)) {
+        ret = create_service_thread("verity-off", std::bind(set_verity_enabled_state_service,
+                                                            std::placeholders::_1, true))
+                      .release();
     } else if (!strcmp(name, "reconnect")) {
-        ret = create_service_thread("reconnect", reconnect_service, transport);
+        ret = create_service_thread("reconnect",
+                                    std::bind(reconnect_service, std::placeholders::_1, transport))
+                      .release();
 #endif
     }
     if (ret >= 0) {
@@ -420,19 +407,16 @@ void connect_emulator(const std::string& port_spec, std::string* response) {
     }
 }
 
-static void connect_service(int fd, void* data) {
-    char* host = reinterpret_cast<char*>(data);
+static void connect_service(android::base::unique_fd fd, std::string host) {
     std::string response;
-    if (!strncmp(host, "emu:", 4)) {
-        connect_emulator(host + 4, &response);
+    if (!strncmp(host.c_str(), "emu:", 4)) {
+        connect_emulator(host.c_str() + 4, &response);
     } else {
-        connect_device(host, &response);
+        connect_device(host.c_str(), &response);
     }
-    free(host);
 
     // Send response for emulator and device
-    SendProtocolString(fd, response);
-    adb_close(fd);
+    SendProtocolString(fd.get(), response);
 }
 #endif
 
@@ -445,7 +429,7 @@ asocket* host_service_to_socket(const char* name, const char* serial, TransportI
     } else if (android::base::StartsWith(name, "wait-for-")) {
         name += strlen("wait-for-");
 
-        std::unique_ptr<state_info> sinfo(new state_info);
+        std::unique_ptr<state_info> sinfo = std::make_unique<state_info>();
         if (sinfo == nullptr) {
             fprintf(stderr, "couldn't allocate state_info: %s", strerror(errno));
             return nullptr;
@@ -481,17 +465,18 @@ asocket* host_service_to_socket(const char* name, const char* serial, TransportI
             return nullptr;
         }
 
-        int fd = create_service_thread("wait", wait_for_state, sinfo.get());
+        int fd = create_service_thread(
+                         "wait", std::bind(wait_for_state, std::placeholders::_1, sinfo.get()))
+                         .release();
         if (fd != -1) {
             sinfo.release();
         }
         return create_local_socket(fd);
     } else if (!strncmp(name, "connect:", 8)) {
-        char* host = strdup(name + 8);
-        int fd = create_service_thread("connect", connect_service, host);
-        if (fd == -1) {
-            free(host);
-        }
+        std::string host(name + strlen("connect:"));
+        int fd = create_service_thread("connect",
+                                       std::bind(connect_service, std::placeholders::_1, host))
+                         .release();
         return create_local_socket(fd);
     }
     return nullptr;

From 6150a37dbe6c683285e98d7bd7ac1abf17ec84b4 Mon Sep 17 00:00:00 2001
From: Luis Hector Chavez <lhchavez@google.com>
Date: Wed, 18 Jul 2018 21:18:27 -0700
Subject: [PATCH 3/3] adb: Remove most C-style allocations

This change gets rid of most malloc/calloc/free calls. The future is
now!

Bug: None
Test: test_device.py
Change-Id: Iccfe3bd4fe45a0319bd9f23b8cbff4c7070c9f4d
---
 adb/adb.cpp                | 27 ++++-------
 adb/adb_listeners.cpp      |  7 +--
 adb/client/commandline.cpp | 22 ++++-----
 adb/sysdeps/memory.h       | 29 ++++++++++++
 adb/transport.cpp          | 94 ++++++++++++++++++--------------------
 adb/transport.h            | 12 ++---
 adb/transport_test.cpp     |  6 +--
 adb/types.h                | 19 ++++----
 8 files changed, 114 insertions(+), 102 deletions(-)

diff --git a/adb/adb.cpp b/adb/adb.cpp
index 20d0db5fb..9b4870280 100644
--- a/adb/adb.cpp
+++ b/adb/adb.cpp
@@ -264,15 +264,6 @@ void send_connect(atransport* t) {
     send_packet(cp, t);
 }
 
-// qual_overwrite is used to overwrite a qualifier string.  dst is a
-// pointer to a char pointer.  It is assumed that if *dst is non-NULL, it
-// was malloc'ed and needs to freed.  *dst will be set to a dup of src.
-// TODO: switch to std::string for these atransport fields instead.
-static void qual_overwrite(char** dst, const std::string& src) {
-    free(*dst);
-    *dst = strdup(src.c_str());
-}
-
 void parse_banner(const std::string& banner, atransport* t) {
     D("parse_banner: %s", banner.c_str());
 
@@ -296,11 +287,11 @@ void parse_banner(const std::string& banner, atransport* t) {
             const std::string& key = key_value[0];
             const std::string& value = key_value[1];
             if (key == "ro.product.name") {
-                qual_overwrite(&t->product, value);
+                t->product = value;
             } else if (key == "ro.product.model") {
-                qual_overwrite(&t->model, value);
+                t->model = value;
             } else if (key == "ro.product.device") {
-                qual_overwrite(&t->device, value);
+                t->device = value;
             } else if (key == "features") {
                 t->SetFeatures(value);
             }
@@ -424,8 +415,8 @@ void handle_packet(apacket *p, atransport *t)
                     /* Other READY messages must use the same local-id */
                     s->ready(s);
                 } else {
-                    D("Invalid A_OKAY(%d,%d), expected A_OKAY(%d,%d) on transport %s",
-                      p->msg.arg0, p->msg.arg1, s->peer->id, p->msg.arg1, t->serial);
+                    D("Invalid A_OKAY(%d,%d), expected A_OKAY(%d,%d) on transport %s", p->msg.arg0,
+                      p->msg.arg1, s->peer->id, p->msg.arg1, t->serial.c_str());
                 }
             } else {
                 // When receiving A_OKAY from device for A_OPEN request, the host server may
@@ -451,8 +442,8 @@ void handle_packet(apacket *p, atransport *t)
                  * socket has a peer on the same transport.
                  */
                 if (p->msg.arg0 == 0 && s->peer && s->peer->transport != t) {
-                    D("Invalid A_CLSE(0, %u) from transport %s, expected transport %s",
-                      p->msg.arg1, t->serial, s->peer->transport->serial);
+                    D("Invalid A_CLSE(0, %u) from transport %s, expected transport %s", p->msg.arg1,
+                      t->serial.c_str(), s->peer->transport->serial.c_str());
                 } else {
                     s->close(s);
                 }
@@ -1171,7 +1162,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser
         std::string error;
         atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error);
         if (t) {
-            return SendOkay(reply_fd, t->serial ? t->serial : "unknown");
+            return SendOkay(reply_fd, !t->serial.empty() ? t->serial : "unknown");
         } else {
             return SendFail(reply_fd, error);
         }
@@ -1180,7 +1171,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser
         std::string error;
         atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error);
         if (t) {
-            return SendOkay(reply_fd, t->devpath ? t->devpath : "unknown");
+            return SendOkay(reply_fd, !t->devpath.empty() ? t->devpath : "unknown");
         } else {
             return SendFail(reply_fd, error);
         }
diff --git a/adb/adb_listeners.cpp b/adb/adb_listeners.cpp
index ea5a44e46..f4a92e360 100644
--- a/adb/adb_listeners.cpp
+++ b/adb/adb_listeners.cpp
@@ -136,9 +136,10 @@ std::string format_listeners() EXCLUDES(listener_list_mutex) {
         }
         //  <device-serial> " " <local-name> " " <remote-name> "\n"
         // Entries from "adb reverse" have no serial.
-        android::base::StringAppendF(&result, "%s %s %s\n",
-                                     l->transport->serial ? l->transport->serial : "(reverse)",
-                                     l->local_name.c_str(), l->connect_to.c_str());
+        android::base::StringAppendF(
+                &result, "%s %s %s\n",
+                !l->transport->serial.empty() ? l->transport->serial.c_str() : "(reverse)",
+                l->local_name.c_str(), l->connect_to.c_str());
     }
     return result;
 }
diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp
index 80d0dd3d1..7791895f6 100644
--- a/adb/client/commandline.cpp
+++ b/adb/client/commandline.cpp
@@ -362,9 +362,8 @@ static void stdinout_raw_epilogue(int inFd, int outFd, int old_stdin_mode, int o
 }
 
 static void copy_to_file(int inFd, int outFd) {
-    const size_t BUFSIZE = 32 * 1024;
-    char* buf = (char*) malloc(BUFSIZE);
-    if (buf == nullptr) fatal("couldn't allocate buffer for copy_to_file");
+    constexpr size_t BUFSIZE = 32 * 1024;
+    std::vector<char> buf(BUFSIZE);
     int len;
     long total = 0;
     int old_stdin_mode = -1;
@@ -376,9 +375,9 @@ static void copy_to_file(int inFd, int outFd) {
 
     while (true) {
         if (inFd == STDIN_FILENO) {
-            len = unix_read(inFd, buf, BUFSIZE);
+            len = unix_read(inFd, buf.data(), BUFSIZE);
         } else {
-            len = adb_read(inFd, buf, BUFSIZE);
+            len = adb_read(inFd, buf.data(), BUFSIZE);
         }
         if (len == 0) {
             D("copy_to_file() : read 0 bytes; exiting");
@@ -389,10 +388,10 @@ static void copy_to_file(int inFd, int outFd) {
             break;
         }
         if (outFd == STDOUT_FILENO) {
-            fwrite(buf, 1, len, stdout);
+            fwrite(buf.data(), 1, len, stdout);
             fflush(stdout);
         } else {
-            adb_write(outFd, buf, len);
+            adb_write(outFd, buf.data(), len);
         }
         total += len;
     }
@@ -400,7 +399,6 @@ static void copy_to_file(int inFd, int outFd) {
     stdinout_raw_epilogue(inFd, outFd, old_stdin_mode, old_stdout_mode);
 
     D("copy_to_file() finished after %lu bytes", total);
-    free(buf);
 }
 
 static void send_window_size_change(int fd, std::unique_ptr<ShellProtocol>& shell) {
@@ -1142,24 +1140,22 @@ static int logcat(int argc, const char** argv) {
 static void write_zeros(int bytes, int fd) {
     int old_stdin_mode = -1;
     int old_stdout_mode = -1;
-    char* buf = (char*) calloc(1, bytes);
-    if (buf == nullptr) fatal("couldn't allocate buffer for write_zeros");
+    std::vector<char> buf(bytes);
 
     D("write_zeros(%d) -> %d", bytes, fd);
 
     stdinout_raw_prologue(-1, fd, old_stdin_mode, old_stdout_mode);
 
     if (fd == STDOUT_FILENO) {
-        fwrite(buf, 1, bytes, stdout);
+        fwrite(buf.data(), 1, bytes, stdout);
         fflush(stdout);
     } else {
-        adb_write(fd, buf, bytes);
+        adb_write(fd, buf.data(), bytes);
     }
 
     stdinout_raw_prologue(-1, fd, old_stdin_mode, old_stdout_mode);
 
     D("write_zeros() finished");
-    free(buf);
 }
 
 static int backup(int argc, const char** argv) {
diff --git a/adb/sysdeps/memory.h b/adb/sysdeps/memory.h
index 0e4c509f6..4108aff87 100644
--- a/adb/sysdeps/memory.h
+++ b/adb/sysdeps/memory.h
@@ -23,6 +23,23 @@
 // We don't have C++14 on Windows yet.
 // Reimplement std::make_unique ourselves until we do.
 
+namespace internal {
+
+template <typename T>
+struct array_known_bounds;
+
+template <typename T>
+struct array_known_bounds<T[]> {
+    constexpr static bool value = false;
+};
+
+template <typename T, size_t N>
+struct array_known_bounds<T[N]> {
+    constexpr static bool value = true;
+};
+
+}  // namespace internal
+
 namespace std {
 
 template <typename T, typename... Args>
@@ -31,6 +48,18 @@ typename std::enable_if<!std::is_array<T>::value, std::unique_ptr<T>>::type make
     return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
 }
 
+template <typename T>
+typename std::enable_if<std::is_array<T>::value && !internal::array_known_bounds<T>::value,
+                        std::unique_ptr<T>>::type
+make_unique(std::size_t size) {
+    return std::unique_ptr<T>(new typename std::remove_extent<T>::type[size]());
+}
+
+template <typename T, typename... Args>
+typename std::enable_if<std::is_array<T>::value && internal::array_known_bounds<T>::value,
+                        std::unique_ptr<T>>::type
+make_unique(Args&&... args) = delete;
+
 }  // namespace std
 
 #endif
diff --git a/adb/transport.cpp b/adb/transport.cpp
index f2c40d2f3..638940c14 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -173,18 +173,18 @@ void ReconnectHandler::Run() {
             attempt = reconnect_queue_.front();
             reconnect_queue_.pop();
             if (attempt.transport->kicked()) {
-                D("transport %s was kicked. giving up on it.", attempt.transport->serial);
+                D("transport %s was kicked. giving up on it.", attempt.transport->serial.c_str());
                 remove_transport(attempt.transport);
                 continue;
             }
         }
-        D("attempting to reconnect %s", attempt.transport->serial);
+        D("attempting to reconnect %s", attempt.transport->serial.c_str());
 
         if (!attempt.transport->Reconnect()) {
-            D("attempting to reconnect %s failed.", attempt.transport->serial);
+            D("attempting to reconnect %s failed.", attempt.transport->serial.c_str());
             if (attempt.attempts_left == 0) {
                 D("transport %s exceeded the number of retry attempts. giving up on it.",
-                  attempt.transport->serial);
+                  attempt.transport->serial.c_str());
                 remove_transport(attempt.transport);
                 continue;
             }
@@ -197,7 +197,7 @@ void ReconnectHandler::Run() {
             continue;
         }
 
-        D("reconnection to %s succeeded.", attempt.transport->serial);
+        D("reconnection to %s succeeded.", attempt.transport->serial.c_str());
         register_transport(attempt.transport);
     }
 }
@@ -402,14 +402,14 @@ void send_packet(apacket* p, atransport* t) {
         p->msg.data_check = calculate_apacket_checksum(p);
     }
 
-    VLOG(TRANSPORT) << dump_packet(t->serial, "to remote", p);
+    VLOG(TRANSPORT) << dump_packet(t->serial.c_str(), "to remote", p);
 
     if (t == nullptr) {
         fatal("Transport is null");
     }
 
     if (t->Write(p) != 0) {
-        D("%s: failed to enqueue packet, closing transport", t->serial);
+        D("%s: failed to enqueue packet, closing transport", t->serial.c_str());
         t->Kick();
     }
 }
@@ -619,19 +619,13 @@ static void transport_registration_func(int _fd, unsigned ev, void*) {
     t = m.transport;
 
     if (m.action == 0) {
-        D("transport: %s deleting", t->serial);
+        D("transport: %s deleting", t->serial.c_str());
 
         {
             std::lock_guard<std::recursive_mutex> lock(transport_lock);
             transport_list.remove(t);
         }
 
-        if (t->product) free(t->product);
-        if (t->serial) free(t->serial);
-        if (t->model) free(t->model);
-        if (t->device) free(t->device);
-        if (t->devpath) free(t->devpath);
-
         delete t;
 
         update_transports();
@@ -646,11 +640,11 @@ static void transport_registration_func(int _fd, unsigned ev, void*) {
         t->connection()->SetTransportName(t->serial_name());
         t->connection()->SetReadCallback([t](Connection*, std::unique_ptr<apacket> p) {
             if (!check_header(p.get(), t)) {
-                D("%s: remote read: bad header", t->serial);
+                D("%s: remote read: bad header", t->serial.c_str());
                 return false;
             }
 
-            VLOG(TRANSPORT) << dump_packet(t->serial, "from remote", p.get());
+            VLOG(TRANSPORT) << dump_packet(t->serial.c_str(), "from remote", p.get());
             apacket* packet = p.release();
 
             // TODO: Does this need to run on the main thread?
@@ -658,7 +652,7 @@ static void transport_registration_func(int _fd, unsigned ev, void*) {
             return true;
         });
         t->connection()->SetErrorCallback([t](Connection*, const std::string& error) {
-            D("%s: connection terminated: %s", t->serial, error.c_str());
+            D("%s: connection terminated: %s", t->serial.c_str(), error.c_str());
             fdevent_run_on_main_thread([t]() {
                 handle_offline(t);
                 transport_unref(t);
@@ -717,7 +711,7 @@ static void register_transport(atransport* transport) {
     tmsg m;
     m.transport = transport;
     m.action = 1;
-    D("transport: %s registered", transport->serial);
+    D("transport: %s registered", transport->serial.c_str());
     if (transport_write_action(transport_registration_send, &m)) {
         fatal_errno("cannot write transport registration socket\n");
     }
@@ -727,7 +721,7 @@ static void remove_transport(atransport* transport) {
     tmsg m;
     m.transport = transport;
     m.action = 0;
-    D("transport: %s removed", transport->serial);
+    D("transport: %s removed", transport->serial.c_str());
     if (transport_write_action(transport_registration_send, &m)) {
         fatal_errno("cannot write transport registration socket\n");
     }
@@ -743,38 +737,38 @@ static void transport_unref(atransport* t) {
     if (t->ref_count == 0) {
         t->connection()->Stop();
         if (t->IsTcpDevice() && !t->kicked()) {
-            D("transport: %s unref (attempting reconnection) %d", t->serial, t->kicked());
+            D("transport: %s unref (attempting reconnection) %d", t->serial.c_str(), t->kicked());
             reconnect_handler.TrackTransport(t);
         } else {
-            D("transport: %s unref (kicking and closing)", t->serial);
+            D("transport: %s unref (kicking and closing)", t->serial.c_str());
             remove_transport(t);
         }
     } else {
-        D("transport: %s unref (count=%zu)", t->serial, t->ref_count);
+        D("transport: %s unref (count=%zu)", t->serial.c_str(), t->ref_count);
     }
 }
 
-static int qual_match(const char* to_test, const char* prefix, const char* qual,
+static int qual_match(const std::string& to_test, const char* prefix, const std::string& qual,
                       bool sanitize_qual) {
-    if (!to_test || !*to_test) /* Return true if both the qual and to_test are null strings. */
-        return !qual || !*qual;
+    if (to_test.empty()) /* Return true if both the qual and to_test are empty strings. */
+        return qual.empty();
 
-    if (!qual) return 0;
+    if (qual.empty()) return 0;
 
+    const char* ptr = to_test.c_str();
     if (prefix) {
         while (*prefix) {
-            if (*prefix++ != *to_test++) return 0;
+            if (*prefix++ != *ptr++) return 0;
         }
     }
 
-    while (*qual) {
-        char ch = *qual++;
+    for (char ch : qual) {
         if (sanitize_qual && !isalnum(ch)) ch = '_';
-        if (ch != *to_test++) return 0;
+        if (ch != *ptr++) return 0;
     }
 
-    /* Everything matched so far.  Return true if *to_test is a NUL. */
-    return !*to_test;
+    /* Everything matched so far.  Return true if *ptr is a NUL. */
+    return !*ptr;
 }
 
 atransport* acquire_one_transport(TransportType type, const char* serial, TransportId transport_id,
@@ -921,7 +915,7 @@ int atransport::Write(apacket* p) {
 
 void atransport::Kick() {
     if (!kicked_.exchange(true)) {
-        D("kicking transport %p %s", this, this->serial);
+        D("kicking transport %p %s", this, this->serial.c_str());
         this->connection()->Stop();
     }
 }
@@ -1040,7 +1034,7 @@ void atransport::RunDisconnects() {
 }
 
 bool atransport::MatchesTarget(const std::string& target) const {
-    if (serial) {
+    if (!serial.empty()) {
         if (target == serial) {
             return true;
         } else if (type == kTransportLocal) {
@@ -1069,10 +1063,9 @@ bool atransport::MatchesTarget(const std::string& target) const {
         }
     }
 
-    return (devpath && target == devpath) ||
-           qual_match(target.c_str(), "product:", product, false) ||
-           qual_match(target.c_str(), "model:", model, true) ||
-           qual_match(target.c_str(), "device:", device, false);
+    return (target == devpath) || qual_match(target, "product:", product, false) ||
+           qual_match(target, "model:", model, true) ||
+           qual_match(target, "device:", device, false);
 }
 
 void atransport::SetConnectionEstablished(bool success) {
@@ -1093,9 +1086,9 @@ static std::string sanitize(std::string str, bool alphanumeric) {
     return str;
 }
 
-static void append_transport_info(std::string* result, const char* key, const char* value,
+static void append_transport_info(std::string* result, const char* key, const std::string& value,
                                   bool alphanumeric) {
-    if (value == nullptr || *value == '\0') {
+    if (value.empty()) {
         return;
     }
 
@@ -1105,8 +1098,8 @@ static void append_transport_info(std::string* result, const char* key, const ch
 }
 
 static void append_transport(const atransport* t, std::string* result, bool long_listing) {
-    const char* serial = t->serial;
-    if (!serial || !serial[0]) {
+    std::string serial = t->serial;
+    if (serial.empty()) {
         serial = "(no serial number)";
     }
 
@@ -1115,7 +1108,8 @@ static void append_transport(const atransport* t, std::string* result, bool long
         *result += '\t';
         *result += t->connection_state_name();
     } else {
-        android::base::StringAppendF(result, "%-22s %s", serial, t->connection_state_name().c_str());
+        android::base::StringAppendF(result, "%-22s %s", serial.c_str(),
+                                     t->connection_state_name().c_str());
 
         append_transport_info(result, "", t->devpath, false);
         append_transport_info(result, "product:", t->product, false);
@@ -1138,7 +1132,7 @@ std::string list_transports(bool long_listing) {
         if (x->type != y->type) {
             return x->type < y->type;
         }
-        return strcmp(x->serial, y->serial) < 0;
+        return x->serial < y->serial;
     });
 
     std::string result;
@@ -1181,7 +1175,7 @@ int register_socket_transport(int s, const char* serial, int port, int local,
 
     std::unique_lock<std::recursive_mutex> lock(transport_lock);
     for (const auto& transport : pending_list) {
-        if (transport->serial && strcmp(serial, transport->serial) == 0) {
+        if (strcmp(serial, transport->serial.c_str()) == 0) {
             VLOG(TRANSPORT) << "socket transport " << transport->serial
                             << " is already in pending_list and fails to register";
             delete t;
@@ -1190,7 +1184,7 @@ int register_socket_transport(int s, const char* serial, int port, int local,
     }
 
     for (const auto& transport : transport_list) {
-        if (transport->serial && strcmp(serial, transport->serial) == 0) {
+        if (strcmp(serial, transport->serial.c_str()) == 0) {
             VLOG(TRANSPORT) << "socket transport " << transport->serial
                             << " is already in transport_list and fails to register";
             delete t;
@@ -1199,7 +1193,7 @@ int register_socket_transport(int s, const char* serial, int port, int local,
     }
 
     pending_list.push_front(t);
-    t->serial = strdup(serial);
+    t->serial = serial;
 
     lock.unlock();
 
@@ -1220,7 +1214,7 @@ atransport* find_transport(const char* serial) {
 
     std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (auto& t : transport_list) {
-        if (t->serial && strcmp(serial, t->serial) == 0) {
+        if (strcmp(serial, t->serial.c_str()) == 0) {
             result = t;
             break;
         }
@@ -1251,11 +1245,11 @@ void register_usb_transport(usb_handle* usb, const char* serial, const char* dev
     D("transport: %p init'ing for usb_handle %p (sn='%s')", t, usb, serial ? serial : "");
     init_usb_transport(t, usb);
     if (serial) {
-        t->serial = strdup(serial);
+        t->serial = serial;
     }
 
     if (devpath) {
-        t->devpath = strdup(devpath);
+        t->devpath = devpath;
     }
 
     {
diff --git a/adb/transport.h b/adb/transport.h
index cb2061510..e9c9d3753 100644
--- a/adb/transport.h
+++ b/adb/transport.h
@@ -238,11 +238,11 @@ class atransport {
     TransportType type = kTransportAny;
 
     // Used to identify transports for clients.
-    char* serial = nullptr;
-    char* product = nullptr;
-    char* model = nullptr;
-    char* device = nullptr;
-    char* devpath = nullptr;
+    std::string serial;
+    std::string product;
+    std::string model;
+    std::string device;
+    std::string devpath;
 
     bool IsTcpDevice() const { return type == kTransportLocal; }
 
@@ -253,7 +253,7 @@ class atransport {
     char token[TOKEN_SIZE] = {};
     size_t failed_auth_attempts = 0;
 
-    std::string serial_name() const { return serial ? serial : "<unknown>"; }
+    std::string serial_name() const { return !serial.empty() ? serial : "<unknown>"; }
     std::string connection_state_name() const;
 
     void update_version(int version, size_t payload);
diff --git a/adb/transport_test.cpp b/adb/transport_test.cpp
index d987d4fa5..8c628d802 100644
--- a/adb/transport_test.cpp
+++ b/adb/transport_test.cpp
@@ -86,9 +86,9 @@ TEST(transport, parse_banner_no_features) {
     ASSERT_EQ(0U, t.features().size());
     ASSERT_EQ(kCsHost, t.GetConnectionState());
 
-    ASSERT_EQ(nullptr, t.product);
-    ASSERT_EQ(nullptr, t.model);
-    ASSERT_EQ(nullptr, t.device);
+    ASSERT_EQ(std::string(), t.product);
+    ASSERT_EQ(std::string(), t.model);
+    ASSERT_EQ(std::string(), t.device);
 }
 
 TEST(transport, parse_banner_product_features) {
diff --git a/adb/types.h b/adb/types.h
index c6b3f0703..a3e5d4842 100644
--- a/adb/types.h
+++ b/adb/types.h
@@ -18,6 +18,7 @@
 
 #include <algorithm>
 #include <deque>
+#include <memory>
 #include <type_traits>
 #include <utility>
 #include <vector>
@@ -37,7 +38,7 @@ struct Block {
 
     template <typename Iterator>
     Block(Iterator begin, Iterator end) : Block(end - begin) {
-        std::copy(begin, end, data_);
+        std::copy(begin, end, data_.get());
     }
 
     Block(const Block& copy) = delete;
@@ -73,11 +74,11 @@ struct Block {
     void assign(InputIt begin, InputIt end) {
         clear();
         allocate(end - begin);
-        std::copy(begin, end, data_);
+        std::copy(begin, end, data_.get());
     }
 
     void clear() {
-        free(data_);
+        data_.reset();
         capacity_ = 0;
         size_ = 0;
     }
@@ -86,11 +87,11 @@ struct Block {
     size_t size() const { return size_; }
     bool empty() const { return size() == 0; }
 
-    char* data() { return data_; }
-    const char* data() const { return data_; }
+    char* data() { return data_.get(); }
+    const char* data() const { return data_.get(); }
 
-    char* begin() { return data_; }
-    const char* begin() const { return data_; }
+    char* begin() { return data_.get(); }
+    const char* begin() const { return data_.get(); }
 
     char* end() { return data() + size_; }
     const char* end() const { return data() + size_; }
@@ -108,13 +109,13 @@ struct Block {
         CHECK_EQ(0ULL, capacity_);
         CHECK_EQ(0ULL, size_);
         if (size != 0) {
-            data_ = static_cast<char*>(malloc(size));
+            data_ = std::make_unique<char[]>(size);
             capacity_ = size;
             size_ = size;
         }
     }
 
-    char* data_ = nullptr;
+    std::unique_ptr<char[]> data_;
     size_t capacity_ = 0;
     size_t size_ = 0;
 };