Merge "[adb] Fix incremental installation on Windows" into rvc-dev

adb/client/commandline.cpp

@@ -1423,13 +1423,13 @@ static bool _is_valid_ack_reply_fd(const int ack_reply_fd) {
 #endif
 }
 
-static bool _is_valid_fd(int fd) {
+static bool _is_valid_os_fd(int fd) {
     // Disallow invalid FDs and stdin/out/err as well.
     if (fd < 3) {
         return false;
     }
 #ifdef _WIN32
-    HANDLE handle = adb_get_os_handle(fd);
+    auto handle = (HANDLE)fd;
     DWORD info = 0;
     if (GetHandleInformation(handle, &info) == 0) {
         return false;
@@ -2005,7 +2005,7 @@ int adb_commandline(int argc, const char** argv) {
 #endif
         }
         int connection_fd = atoi(argv[1]);
-        if (!_is_valid_fd(connection_fd)) {
+        if (!_is_valid_os_fd(connection_fd)) {
             error_exit("Invalid connection_fd number given: %d", connection_fd);
         }
 
@@ -2013,7 +2013,7 @@ int adb_commandline(int argc, const char** argv) {
         close_on_exec(connection_fd);
 
         int output_fd = atoi(argv[2]);
-        if (!_is_valid_fd(output_fd)) {
+        if (!_is_valid_os_fd(output_fd)) {
             error_exit("Invalid output_fd number given: %d", output_fd);
         }
         output_fd = adb_register_socket(output_fd);

adb/client/incremental.cpp

@@ -41,8 +41,7 @@ using Size = int64_t;
 
 static inline int32_t read_int32(borrowed_fd fd) {
     int32_t result;
-    ReadFully(fd, &result, sizeof(result));
-    return result;
+    return ReadFdExactly(fd, &result, sizeof(result)) ? result : -1;
 }
 
 static inline void append_int(borrowed_fd fd, std::vector<char>* bytes) {
@@ -54,11 +53,14 @@ static inline void append_int(borrowed_fd fd, std::vector<char>* bytes) {
 
 static inline void append_bytes_with_size(borrowed_fd fd, std::vector<char>* bytes) {
     int32_t le_size = read_int32(fd);
+    if (le_size < 0) {
+        return;
+    }
     int32_t size = int32_t(le32toh(le_size));
     auto old_size = bytes->size();
     bytes->resize(old_size + sizeof(le_size) + size);
     memcpy(bytes->data() + old_size, &le_size, sizeof(le_size));
-    ReadFully(fd, bytes->data() + old_size + sizeof(le_size), size);
+    ReadFdExactly(fd, bytes->data() + old_size + sizeof(le_size), size);
 }
 
 static inline std::pair<std::vector<char>, int32_t> read_id_sig_headers(borrowed_fd fd) {
@@ -200,7 +202,7 @@ std::optional<Process> install(std::vector<std::string> files) {
         return {};
     }
     auto [pipe_read_fd, pipe_write_fd] = print_fds;
-    auto pipe_write_fd_param = std::to_string(pipe_write_fd);
+    auto pipe_write_fd_param = std::to_string(intptr_t(adb_get_os_handle(pipe_write_fd)));
     close_on_exec(pipe_read_fd);
 
     std::vector<std::string> args(std::move(files));

adb/client/incremental_utils.cpp

@@ -18,6 +18,7 @@
 
 #include "incremental_utils.h"
 
+#include <android-base/mapped_file.h>
 #include <android-base/strings.h>
 #include <ziparchive/zip_archive.h>
 #include <ziparchive/zip_writer.h>
@@ -26,6 +27,7 @@
 #include <numeric>
 #include <unordered_set>
 
+#include "adb_trace.h"
 #include "sysdeps.h"
 
 static constexpr int kBlockSize = 4096;
@@ -155,18 +157,81 @@ static std::vector<int32_t> ZipPriorityBlocks(off64_t signerBlockOffset, int64_t
     return zipPriorityBlocks;
 }
 
-// TODO(b/151676293): avoid using OpenArchiveFd that reads local file headers
+[[maybe_unused]] static ZipArchiveHandle openZipArchiveFd(int fd) {
+    bool transferFdOwnership = false;
+#ifdef _WIN32
+    //
+    // Need to create a special CRT FD here as the current one is not compatible with
+    // normal read()/write() calls that libziparchive uses.
+    // To make this work we have to create a copy of the file handle, as CRT doesn't care
+    // and closes it together with the new descriptor.
+    //
+    // Note: don't move this into a helper function, it's better to be hard to reuse because
+    //       the code is ugly and won't work unless it's a last resort.
+    //
+    auto handle = adb_get_os_handle(fd);
+    HANDLE dupedHandle;
+    if (!::DuplicateHandle(::GetCurrentProcess(), handle, ::GetCurrentProcess(), &dupedHandle, 0,
+                           false, DUPLICATE_SAME_ACCESS)) {
+        D("%s failed at DuplicateHandle: %d", __func__, (int)::GetLastError());
+        return {};
+    }
+    fd = _open_osfhandle((intptr_t)dupedHandle, _O_RDONLY | _O_BINARY);
+    if (fd < 0) {
+        D("%s failed at _open_osfhandle: %d", __func__, errno);
+        ::CloseHandle(handle);
+        return {};
+    }
+    transferFdOwnership = true;
+#endif
+    ZipArchiveHandle zip;
+    if (OpenArchiveFd(fd, "apk_fd", &zip, transferFdOwnership) != 0) {
+        D("%s failed at OpenArchiveFd: %d", __func__, errno);
+#ifdef _WIN32
+        // "_close()" is a secret WinCRT name for the regular close() function.
+        _close(fd);
+#endif
+        return {};
+    }
+    return zip;
+}
+
+static std::pair<ZipArchiveHandle, std::unique_ptr<android::base::MappedFile>> openZipArchive(
+        int fd, int64_t fileSize) {
+#ifndef __LP64__
+    if (fileSize >= INT_MAX) {
+        return {openZipArchiveFd(fd), nullptr};
+    }
+#endif
+    auto mapping =
+            android::base::MappedFile::FromOsHandle(adb_get_os_handle(fd), 0, fileSize, PROT_READ);
+    if (!mapping) {
+        D("%s failed at FromOsHandle: %d", __func__, errno);
+        return {};
+    }
+    ZipArchiveHandle zip;
+    if (OpenArchiveFromMemory(mapping->data(), mapping->size(), "apk_mapping", &zip) != 0) {
+        D("%s failed at OpenArchiveFromMemory: %d", __func__, errno);
+        return {};
+    }
+    return {zip, std::move(mapping)};
+}
+
+// TODO(b/151676293): avoid using libziparchive as it reads local file headers
 // which causes additional performance cost. Instead, only read from central directory.
 static std::vector<int32_t> InstallationPriorityBlocks(int fd, int64_t fileSize) {
-    std::vector<int32_t> installationPriorityBlocks;
-    ZipArchiveHandle zip;
-    if (OpenArchiveFd(fd, "", &zip, false) != 0) {
+    auto [zip, _] = openZipArchive(fd, fileSize);
+    if (!zip) {
         return {};
     }
+
     void* cookie = nullptr;
     if (StartIteration(zip, &cookie) != 0) {
+        D("%s failed at StartIteration: %d", __func__, errno);
         return {};
     }
+
+    std::vector<int32_t> installationPriorityBlocks;
     ZipEntry entry;
     std::string_view entryName;
     while (Next(cookie, &entry, &entryName) == 0) {
@@ -183,6 +248,7 @@ static std::vector<int32_t> InstallationPriorityBlocks(int fd, int64_t fileSize)
             int32_t endBlockIndex = offsetToBlockIndex(entryEndOffset);
             int32_t numNewBlocks = endBlockIndex - startBlockIndex + 1;
             appendBlocks(startBlockIndex, numNewBlocks, &installationPriorityBlocks);
+            D("\tadding to priority blocks: '%.*s'", (int)entryName.size(), entryName.data());
         } else if (entryName == "classes.dex") {
             // Only the head is needed for installation
             int32_t startBlockIndex = offsetToBlockIndex(entry.offset);
@@ -213,4 +279,4 @@ std::vector<int32_t> PriorityBlocksForFile(const std::string& filepath, int fd,
     unduplicate(priorityBlocks);
     return priorityBlocks;
 }
-}  // namespace incremental
+}  // namespace incremental

adb/sysdeps.h

@@ -276,6 +276,7 @@ inline void seekdir(DIR*, long) {
 class Process {
   public:
     constexpr explicit Process(HANDLE h = nullptr) : h_(h) {}
+    constexpr Process(Process&& other) : h_(std::exchange(other.h_, nullptr)) {}
     ~Process() { close(); }
     constexpr explicit operator bool() const { return h_ != nullptr; }
 
@@ -292,6 +293,8 @@ class Process {
     }
 
   private:
+    DISALLOW_COPY_AND_ASSIGN(Process);
+
     void close() {
         if (*this) {
             ::CloseHandle(h_);
@@ -666,6 +669,8 @@ static __inline__ int adb_get_os_handle(borrowed_fd fd) {
 class Process {
   public:
     constexpr explicit Process(pid_t pid) : pid_(pid) {}
+    constexpr Process(Process&& other) : pid_(std::exchange(other.pid_, -1)) {}
+
     constexpr explicit operator bool() const { return pid_ >= 0; }
 
     void wait() {
@@ -682,6 +687,8 @@ class Process {
     }
 
   private:
+    DISALLOW_COPY_AND_ASSIGN(Process);
+
     pid_t pid_;
 };