From 6150a37dbe6c683285e98d7bd7ac1abf17ec84b4 Mon Sep 17 00:00:00 2001 From: Luis Hector Chavez Date: Wed, 18 Jul 2018 21:18:27 -0700 Subject: [PATCH] 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) { } // " " " " "\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 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& 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 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 +struct array_known_bounds; + +template +struct array_known_bounds { + constexpr static bool value = false; +}; + +template +struct array_known_bounds { + constexpr static bool value = true; +}; + +} // namespace internal + namespace std { template @@ -31,6 +48,18 @@ typename std::enable_if::value, std::unique_ptr>::type make return std::unique_ptr(new T(std::forward(args)...)); } +template +typename std::enable_if::value && !internal::array_known_bounds::value, + std::unique_ptr>::type +make_unique(std::size_t size) { + return std::unique_ptr(new typename std::remove_extent::type[size]()); +} + +template +typename std::enable_if::value && internal::array_known_bounds::value, + std::unique_ptr>::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 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 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 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 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 : ""; } + std::string serial_name() const { return !serial.empty() ? serial : ""; } 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 #include +#include #include #include #include @@ -37,7 +38,7 @@ struct Block { template 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(malloc(size)); + data_ = std::make_unique(size); capacity_ = size; size_ = size; } } - char* data_ = nullptr; + std::unique_ptr data_; size_t capacity_ = 0; size_t size_ = 0; };