adb: fix adb install and adb push exit code, error handling, unittest

adb push was not returning a bad exit code when write_data_file() or
write_data_link() failed. I encountered this when running the unittest
on Windows which can get into situations where stat() succeeds, but
open() fails due to pre-existing exclusive file access (which typically
doesn't exist on unix).

The same code is used by adb install, so this also fixes its error
handling.

Fixed some fd leaks and propagation of errors when reading a file.

Fixed a unittest to close temp files before reading them.

Change-Id: Ieba0026fa4c79eb0484676e4f2faaac9603ef584
Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>

adb/file_sync_client.cpp

@@ -179,39 +179,41 @@ static bool sync_stat(SyncConnection& sc, const char* path,
     return sync_start_stat(sc, path) && sync_finish_stat(sc, timestamp, mode, size);
 }
 
-static int write_data_file(SyncConnection& sc, const char* path, syncsendbuf* sbuf, bool show_progress) {
-    int err = 0;
+static bool write_data_file(SyncConnection& sc, const char* path, syncsendbuf* sbuf, bool show_progress) {
     unsigned long long size = 0;
+    if (show_progress) {
+        // Determine local file size.
+        struct stat st;
+        if (stat(path, &st) == -1) {
+            fprintf(stderr, "cannot stat '%s': %s\n", path, strerror(errno));
+            return false;
+        }
+
+        size = st.st_size;
+    }
 
     int lfd = adb_open(path, O_RDONLY);
     if (lfd < 0) {
         fprintf(stderr, "cannot open '%s': %s\n", path, strerror(errno));
-        return -1;
-    }
-
-    if (show_progress) {
-        // Determine local file size.
-        struct stat st;
-        if (stat(path, &st)) {
-            fprintf(stderr, "cannot stat '%s': %s\n", path, strerror(errno));
-            return -1;
-        }
-
-        size = st.st_size;
+        return false;
     }
 
     sbuf->id = ID_DATA;
     while (true) {
         int ret = adb_read(lfd, sbuf->data, sc.max);
         if (ret <= 0) {
-            if (ret < 0) fprintf(stderr, "cannot read '%s': %s\n", path, strerror(errno));
+            if (ret < 0) {
+                fprintf(stderr, "cannot read '%s': %s\n", path, strerror(errno));
+                adb_close(lfd);
+                return false;
+            }
             break;
         }
 
         sbuf->size = ret;
         if (!WriteFdExactly(sc.fd, sbuf, sizeof(unsigned) * 2 + ret)) {
-            err = -1;
-            break;
+            adb_close(lfd);
+            return false;
         }
         sc.total_bytes += ret;
 
@@ -221,17 +223,17 @@ static int write_data_file(SyncConnection& sc, const char* path, syncsendbuf* sb
     }
 
     adb_close(lfd);
-    return err;
+    return true;
 }
 
 #if defined(_WIN32)
-extern int write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) __attribute__((error("no symlinks on Windows")));
+extern bool write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) __attribute__((error("no symlinks on Windows")));
 #else
-static int write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) {
+static bool write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sbuf) {
     ssize_t len = readlink(path, sbuf->data, sc.max - 1);
     if (len < 0) {
         fprintf(stderr, "error reading link '%s': %s\n", path, strerror(errno));
-        return -1;
+        return false;
     }
     sbuf->data[len] = '\0';
 
@@ -239,12 +241,12 @@ static int write_data_link(SyncConnection& sc, const char* path, syncsendbuf* sb
     sbuf->id = ID_DATA;
 
     if (!WriteFdExactly(sc.fd, sbuf, sizeof(unsigned) * 2 + len + 1)) {
-        return -1;
+        return false;
     }
 
     sc.total_bytes += len + 1;
 
-    return 0;
+    return true;
 }
 #endif
 
@@ -257,9 +259,9 @@ static bool sync_send(SyncConnection& sc, const char *lpath, const char *rpath,
     if (!SendRequest(sc.fd, ID_SEND, path_and_mode.c_str())) goto fail;
 
     if (S_ISREG(mode)) {
-        write_data_file(sc, lpath, sbuf, show_progress);
+        if (!write_data_file(sc, lpath, sbuf, show_progress)) return false;
     } else if (S_ISLNK(mode)) {
-        write_data_link(sc, lpath, sbuf);
+        if (!write_data_link(sc, lpath, sbuf)) return false;
     } else {
         goto fail;
     }
@@ -325,6 +327,7 @@ static int sync_recv(SyncConnection& sc, const char* rpath, const char* lpath, b
 
     while (true) {
         if(!ReadFdExactly(sc.fd, &msg.data, sizeof(msg.data))) {
+            adb_close(lfd);
             return -1;
         }
         id = msg.data.id;

adb/test_device.py

@@ -215,12 +215,18 @@ class ArgumentEscapingTest(DeviceTest):
     def test_install_argument_escaping(self):
         """Make sure that install argument escaping works."""
         # http://b/20323053
-        tf = tempfile.NamedTemporaryFile('wb', suffix='-text;ls;1.apk')
+        tf = tempfile.NamedTemporaryFile('wb', suffix='-text;ls;1.apk',
+                                         delete=False)
+        tf.close()
         self.assertIn("-text;ls;1.apk", self.device.install(tf.name))
+        os.remove(tf.name)
 
         # http://b/3090932
-        tf = tempfile.NamedTemporaryFile('wb', suffix="-Live Hold'em.apk")
+        tf = tempfile.NamedTemporaryFile('wb', suffix="-Live Hold'em.apk",
+                                         delete=False)
+        tf.close()
         self.assertIn("-Live Hold'em.apk", self.device.install(tf.name))
+        os.remove(tf.name)
 
 
 class RootUnrootTest(DeviceTest):