From 03de645aac4dac3b19900b3d30a29b2d10249ad6 Mon Sep 17 00:00:00 2001
From: David Anderson <dvander@google.com>
Date: Tue, 4 Sep 2018 14:32:54 -0700
Subject: [PATCH] fastbootd: Fix transport ownership.
This change moves Transport ownership back out of FastBootDriver.
Callers of set_transport must ensure that the previous transport is
destroyed. In addition, deleting a transport now ensures that it is
closed.
Bug: 78793464
Test: fastboot, fuzzy_fastboot works
Change-Id: I8f9ed2f7d5b09fd0820b2677d087a027378f26db
---
fastboot/engine.cpp | 6 ++++--
fastboot/fastboot.cpp | 4 ++++
fastboot/fastboot_driver.cpp | 10 +++-------
fastboot/fastboot_driver.h | 4 ++--
fastboot/fuzzy_fastboot/fixtures.cpp | 3 ---
fastboot/fuzzy_fastboot/usb_transport_sniffer.cpp | 4 ++++
fastboot/fuzzy_fastboot/usb_transport_sniffer.h | 1 +
fastboot/usb_linux.cpp | 6 +++++-
fastboot/usb_osx.cpp | 6 +++++-
fastboot/usb_windows.cpp | 6 +++++-
10 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/fastboot/engine.cpp b/fastboot/engine.cpp
index 6a52b12bbd..d80e98601e 100644
--- a/fastboot/engine.cpp
+++ b/fastboot/engine.cpp
@@ -88,7 +88,9 @@ void fb_init(fastboot::FastBootDriver& fbi) {
}
void fb_reinit(Transport* transport) {
- fb->set_transport(transport);
+ if (Transport* old_transport = fb->set_transport(transport)) {
+ delete old_transport;
+ }
}
const std::string fb_get_error() {
@@ -392,6 +394,6 @@ bool fb_reboot_to_userspace() {
}
fprintf(stderr, "OKAY\n");
- fb->set_transport(nullptr);
+ fb_reinit(nullptr);
return true;
}
diff --git a/fastboot/fastboot.cpp b/fastboot/fastboot.cpp
index 2887d3b5d8..1aef567f20 100644
--- a/fastboot/fastboot.cpp
+++ b/fastboot/fastboot.cpp
@@ -1847,6 +1847,10 @@ int FastBootTool::Main(int argc, char* argv[]) {
int status = fb_execute_queue() ? EXIT_FAILURE : EXIT_SUCCESS;
fprintf(stderr, "Finished. Total time: %.3fs\n", (now() - start));
+
+ if (Transport* old_transport = fb.set_transport(nullptr)) {
+ delete old_transport;
+ }
return status;
}
diff --git a/fastboot/fastboot_driver.cpp b/fastboot/fastboot_driver.cpp
index ceee066c9b..6bd61282b7 100644
--- a/fastboot/fastboot_driver.cpp
+++ b/fastboot/fastboot_driver.cpp
@@ -58,7 +58,6 @@ FastBootDriver::FastBootDriver(Transport* transport, std::function<void(std::str
}
FastBootDriver::~FastBootDriver() {
- set_transport(nullptr);
}
RetCode FastBootDriver::Boot(std::string* response, std::vector<std::string>* info) {
@@ -537,12 +536,9 @@ int FastBootDriver::SparseWriteCallback(std::vector<char>& tpbuf, const char* da
return 0;
}
-void FastBootDriver::set_transport(Transport* transport) {
- if (transport_) {
- transport_->Close();
- delete transport_;
- }
- transport_ = transport;
+Transport* FastBootDriver::set_transport(Transport* transport) {
+ std::swap(transport_, transport);
+ return transport;
}
} // End namespace fastboot
diff --git a/fastboot/fastboot_driver.h b/fastboot/fastboot_driver.h
index 464794538d..2651690cb9 100644
--- a/fastboot/fastboot_driver.h
+++ b/fastboot/fastboot_driver.h
@@ -109,8 +109,8 @@ class FastBootDriver {
std::string Error();
RetCode WaitForDisconnect();
- // Note: changing the transport will close and delete the existing one.
- void set_transport(Transport* transport);
+ // Note: set_transport will return the previous transport.
+ Transport* set_transport(Transport* transport);
Transport* transport() const { return transport_; }
// This is temporarily public for engine.cpp
diff --git a/fastboot/fuzzy_fastboot/fixtures.cpp b/fastboot/fuzzy_fastboot/fixtures.cpp
index 0a8759830d..4da71ca587 100644
--- a/fastboot/fuzzy_fastboot/fixtures.cpp
+++ b/fastboot/fuzzy_fastboot/fixtures.cpp
@@ -133,7 +133,6 @@ void FastBootTest::TearDown() {
fb.reset();
if (transport) {
- transport->Close();
transport.reset();
}
@@ -188,7 +187,6 @@ void FastBootTest::SetLockState(bool unlock, bool assert_change) {
ASSERT_EQ(fb->RawCommand("flashing " + cmd, &resp), SUCCESS)
<< "Attempting to change locked state, but 'flashing" + cmd + "' command failed";
fb.reset();
- transport->Close();
transport.reset();
printf("PLEASE RESPOND TO PROMPT FOR '%sing' BOOTLOADER ON DEVICE\n", cmd.c_str());
while (UsbStillAvailible())
@@ -249,7 +247,6 @@ void Fuzz::TearDown() {
}
if (transport) {
- transport->Close();
transport.reset();
}
diff --git a/fastboot/fuzzy_fastboot/usb_transport_sniffer.cpp b/fastboot/fuzzy_fastboot/usb_transport_sniffer.cpp
index ee510e9185..7c595f47da 100644
--- a/fastboot/fuzzy_fastboot/usb_transport_sniffer.cpp
+++ b/fastboot/fuzzy_fastboot/usb_transport_sniffer.cpp
@@ -12,6 +12,10 @@ UsbTransportSniffer::UsbTransportSniffer(std::unique_ptr<UsbTransport> transport
const int serial_fd)
: transport_(std::move(transport)), serial_fd_(serial_fd) {}
+UsbTransportSniffer::~UsbTransportSniffer() {
+ Close();
+}
+
ssize_t UsbTransportSniffer::Read(void* data, size_t len) {
ProcessSerial();
diff --git a/fastboot/fuzzy_fastboot/usb_transport_sniffer.h b/fastboot/fuzzy_fastboot/usb_transport_sniffer.h
index 693f04282b..89cc00993d 100644
--- a/fastboot/fuzzy_fastboot/usb_transport_sniffer.h
+++ b/fastboot/fuzzy_fastboot/usb_transport_sniffer.h
@@ -68,6 +68,7 @@ class UsbTransportSniffer : public UsbTransport {
};
UsbTransportSniffer(std::unique_ptr<UsbTransport> transport, const int serial_fd = 0);
+ ~UsbTransportSniffer() override;
virtual ssize_t Read(void* data, size_t len) override;
virtual ssize_t Write(const void* data, size_t len) override;
diff --git a/fastboot/usb_linux.cpp b/fastboot/usb_linux.cpp
index 9b779ddea6..6363aa547a 100644
--- a/fastboot/usb_linux.cpp
+++ b/fastboot/usb_linux.cpp
@@ -95,7 +95,7 @@ class LinuxUsbTransport : public UsbTransport {
public:
explicit LinuxUsbTransport(std::unique_ptr<usb_handle> handle, uint32_t ms_timeout = 0)
: handle_(std::move(handle)), ms_timeout_(ms_timeout) {}
- ~LinuxUsbTransport() override = default;
+ ~LinuxUsbTransport() override;
ssize_t Read(void* data, size_t len) override;
ssize_t Write(const void* data, size_t len) override;
@@ -387,6 +387,10 @@ static std::unique_ptr<usb_handle> find_usb_device(const char* base, ifc_match_f
return usb;
}
+LinuxUsbTransport::~LinuxUsbTransport() {
+ Close();
+}
+
ssize_t LinuxUsbTransport::Write(const void* _data, size_t len)
{
unsigned char *data = (unsigned char*) _data;
diff --git a/fastboot/usb_osx.cpp b/fastboot/usb_osx.cpp
index 4d48f6e26b..ed02c4a288 100644
--- a/fastboot/usb_osx.cpp
+++ b/fastboot/usb_osx.cpp
@@ -70,7 +70,7 @@ class OsxUsbTransport : public UsbTransport {
// A timeout of 0 is blocking
OsxUsbTransport(std::unique_ptr<usb_handle> handle, uint32_t ms_timeout = 0)
: handle_(std::move(handle)), ms_timeout_(ms_timeout) {}
- ~OsxUsbTransport() override = default;
+ ~OsxUsbTransport() override;
ssize_t Read(void* data, size_t len) override;
ssize_t Write(const void* data, size_t len) override;
@@ -471,6 +471,10 @@ UsbTransport* usb_open(ifc_match_func callback, uint32_t timeout_ms) {
return new OsxUsbTransport(std::move(handle), timeout_ms);
}
+OsxUsbTransport::~OsxUsbTransport() {
+ Close();
+}
+
int OsxUsbTransport::Close() {
/* TODO: Something better here? */
return 0;
diff --git a/fastboot/usb_windows.cpp b/fastboot/usb_windows.cpp
index 8c60a71737..b00edb3fdd 100644
--- a/fastboot/usb_windows.cpp
+++ b/fastboot/usb_windows.cpp
@@ -69,7 +69,7 @@ struct usb_handle {
class WindowsUsbTransport : public UsbTransport {
public:
WindowsUsbTransport(std::unique_ptr<usb_handle> handle) : handle_(std::move(handle)) {}
- ~WindowsUsbTransport() override = default;
+ ~WindowsUsbTransport() override;
ssize_t Read(void* data, size_t len) override;
ssize_t Write(const void* data, size_t len) override;
@@ -250,6 +250,10 @@ void usb_kick(usb_handle* handle) {
}
}
+WindowsUsbTransport::~WindowsUsbTransport() {
+ Close();
+}
+
int WindowsUsbTransport::Close() {
DBG("usb_close\n");
--
GitLab