diff --git a/adb/file_sync_client.cpp b/adb/file_sync_client.cpp index 51fc1433d..85aaa6141 100644 --- a/adb/file_sync_client.cpp +++ b/adb/file_sync_client.cpp @@ -88,7 +88,8 @@ class SyncConnection { : total_bytes_(0), start_time_ms_(CurrentTimeMs()), expected_total_bytes_(0), - expect_multiple_files_(false) { + expect_multiple_files_(false), + expect_done_(false) { max = SYNC_DATA_MAX; // TODO: decide at runtime. std::string error; @@ -117,6 +118,16 @@ class SyncConnection { bool IsValid() { return fd >= 0; } + bool ReceivedError(const char* from, const char* to) { + adb_pollfd pfd = {.fd = fd, .events = POLLIN}; + int rc = adb_poll(&pfd, 1, 0); + if (rc < 0) { + Error("failed to poll: %s", strerror(errno)); + return true; + } + return rc != 0; + } + bool SendRequest(int id, const char* path_and_mode) { size_t path_length = strlen(path_and_mode); if (path_length > 1024) { @@ -175,6 +186,7 @@ class SyncConnection { p += sizeof(SyncRequest); WriteOrDie(lpath, rpath, &buf[0], (p - &buf[0])); + expect_done_ = true; total_bytes_ += data_length; return true; } @@ -220,6 +232,11 @@ class SyncConnection { total_bytes_ += bytes_read; bytes_copied += bytes_read; + // Check to see if we've received an error from the other side. + if (ReceivedError(lpath, rpath)) { + break; + } + ReportProgress(rpath, bytes_copied, total_size); } @@ -228,17 +245,24 @@ class SyncConnection { syncmsg msg; msg.data.id = ID_DONE; msg.data.size = mtime; + expect_done_ = true; return WriteOrDie(lpath, rpath, &msg.data, sizeof(msg.data)); } bool CopyDone(const char* from, const char* to) { syncmsg msg; if (!ReadFdExactly(fd, &msg.status, sizeof(msg.status))) { - Error("failed to copy '%s' to '%s': no ID_DONE: %s", from, to, strerror(errno)); + Error("failed to copy '%s' to '%s': couldn't read from device", from, to); return false; } if (msg.status.id == ID_OKAY) { - return true; + if (expect_done_) { + expect_done_ = false; + return true; + } else { + Error("failed to copy '%s' to '%s': received premature success", from, to); + return true; + } } if (msg.status.id != ID_FAIL) { Error("failed to copy '%s' to '%s': unknown reason %d", from, to, msg.status.id); @@ -357,6 +381,7 @@ class SyncConnection { uint64_t expected_total_bytes_; bool expect_multiple_files_; + bool expect_done_; LinePrinter line_printer_; diff --git a/adb/file_sync_service.cpp b/adb/file_sync_service.cpp index 29c662929..926dbcfa9 100644 --- a/adb/file_sync_service.cpp +++ b/adb/file_sync_service.cpp @@ -183,8 +183,6 @@ static bool handle_send_file(int s, const char* path, uid_t uid, } while (true) { - unsigned int len; - if (!ReadFdExactly(s, &msg.data, sizeof(msg.data))) goto fail; if (msg.data.id != ID_DATA) { @@ -193,17 +191,17 @@ static bool handle_send_file(int s, const char* path, uid_t uid, break; } SendSyncFail(s, "invalid data message"); - goto fail; + goto abort; } - len = msg.data.size; - if (len > buffer.size()) { // TODO: resize buffer? + + if (msg.data.size > buffer.size()) { // TODO: resize buffer? SendSyncFail(s, "oversize data message"); - goto fail; + goto abort; } - if (!ReadFdExactly(s, &buffer[0], len)) goto fail; + if (!ReadFdExactly(s, &buffer[0], msg.data.size)) goto abort; - if (!WriteFdExactly(fd, &buffer[0], len)) { + if (!WriteFdExactly(fd, &buffer[0], msg.data.size)) { SendSyncFailErrno(s, "write failed"); goto fail; } @@ -221,6 +219,35 @@ static bool handle_send_file(int s, const char* path, uid_t uid, return WriteFdExactly(s, &msg.status, sizeof(msg.status)); fail: + // If there's a problem on the device, we'll send an ID_FAIL message and + // close the socket. Unfortunately the kernel will sometimes throw that + // data away if the other end keeps writing without reading (which is + // the case with old versions of adb). To maintain compatibility, keep + // reading and throwing away ID_DATA packets until the other side notices + // that we've reported an error. + while (true) { + if (!ReadFdExactly(s, &msg.data, sizeof(msg.data))) goto fail; + + if (msg.data.id == ID_DONE) { + goto abort; + } else if (msg.data.id != ID_DATA) { + char id[5]; + memcpy(id, &msg.data.id, sizeof(msg.data.id)); + id[4] = '\0'; + D("handle_send_fail received unexpected id '%s' during failure", id); + goto abort; + } + + if (msg.data.size > buffer.size()) { + D("handle_send_fail received oversized packet of length '%u' during failure", + msg.data.size); + goto abort; + } + + if (!ReadFdExactly(s, &buffer[0], msg.data.size)) goto abort; + } + +abort: if (fd >= 0) adb_close(fd); if (do_unlink) adb_unlink(path); return false; @@ -403,18 +430,6 @@ static bool handle_sync_command(int fd, std::vector& buffer) { void file_sync_service(int fd, void* cookie) { std::vector buffer(SYNC_DATA_MAX); - // If there's a problem on the device, we'll send an ID_FAIL message and - // close the socket. Unfortunately the kernel will sometimes throw that - // data away if the other end keeps writing without reading (which is - // the normal case with our protocol --- they won't read until the end). - // So set SO_LINGER to give the client 20s to get around to reading our - // failure response. Without this, the other side's ability to report - // useful errors is reduced. - struct linger l; - l.l_onoff = 1; - l.l_linger = 20; - adb_setsockopt(fd, SOL_SOCKET, SO_LINGER, &l, sizeof(l)); - while (handle_sync_command(fd, buffer)) { } diff --git a/adb/test_device.py b/adb/test_device.py index afc061a60..18174a2ae 100644 --- a/adb/test_device.py +++ b/adb/test_device.py @@ -766,6 +766,22 @@ class FileOperationsTest(DeviceTest): if host_dir is not None: shutil.rmtree(host_dir) + @requires_non_root + def test_push_error_reporting(self): + """Make sure that errors that occur while pushing a file get reported + + Bug: http://b/26816782 + """ + with tempfile.NamedTemporaryFile() as tmp_file: + tmp_file.write('\0' * 1024 * 1024) + tmp_file.flush() + try: + self.device.push(local=tmp_file.name, remote='/system/') + self.fail("push should not have succeeded") + except subprocess.CalledProcessError as e: + output = e.output + + self.assertIn("Permission denied", output) def _test_pull(self, remote_file, checksum): tmp_write = tempfile.NamedTemporaryFile(mode='wb', delete=False)