commit 350710fb3d92d78bcbb36087032a3ffb03c4ad0e
author Josh Gao <jmgao@google.com> Tue Apr 04 13:43:21 2017 -0700
committer Josh Gao <jmgao@google.com> Tue Apr 04 21:22:09 2017 -0700
tree e08c419e9c197d7d4f082471009a29a11ad29c9c
parent 95e58846dce255880cf9a99dd29fdafffdad60cd

debuggerd_client: fix infinite timeouts.

Bug: http://b/36897117
Test: debuggerd_test
Test: debuggerd -b `pidof zygote`
Change-Id: I743433bc420b1c433b82ef62795abb6ea9f5d840
(cherry picked from commit 287d50dc590cc00642783476d33e569066c0fcca)

debuggerd/client/debuggerd_client.cpp

diff --git a/debuggerd/client/debuggerd_client.cpp b/debuggerd/client/debuggerd_client.cpp
index 224444f..3b84853 100644
--- a/debuggerd/client/debuggerd_client.cpp
+++ b/debuggerd/client/debuggerd_client.cpp
@@ -58,28 +58,31 @@
 }
 
 bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType dump_type,
-                            int timeout_ms) {
+                            unsigned int timeout_ms) {
   LOG(INFO) << "libdebuggerd_client: started dumping process " << pid;
   unique_fd sockfd;
   const auto end = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeout_ms);
-  auto time_left = [timeout_ms, &end]() { return end - std::chrono::steady_clock::now(); };
+  auto time_left = [&end]() { return end - std::chrono::steady_clock::now(); };
   auto set_timeout = [timeout_ms, &time_left](int sockfd) {
     if (timeout_ms <= 0) {
-      return -1;
+      return sockfd;
     }
 
     auto remaining = time_left();
     if (remaining < decltype(remaining)::zero()) {
-      LOG(ERROR) << "timeout expired";
+      LOG(ERROR) << "libdebuggerd_client: timeout expired";
       return -1;
     }
+
     struct timeval timeout;
     populate_timeval(&timeout, remaining);
 
     if (setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)) != 0) {
+      PLOG(ERROR) << "libdebuggerd_client: failed to set receive timeout";
       return -1;
     }
     if (setsockopt(sockfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)) != 0) {
+      PLOG(ERROR) << "libdebuggerd_client: failed to set send timeout";
       return -1;
     }
 
@@ -158,8 +161,10 @@
 
   // Forward output from the pipe to the output fd.
   while (true) {
-    auto remaining_ms = std::chrono::duration_cast<std::chrono::milliseconds>(time_left());
-    if (remaining_ms <= 1ms) {
+    auto remaining_ms = std::chrono::duration_cast<std::chrono::milliseconds>(time_left()).count();
+    if (timeout_ms <= 0) {
+      remaining_ms = -1;
+    } else if (remaining_ms < 0) {
       LOG(ERROR) << "libdebuggerd_client: timeout expired";
       return false;
     }
@@ -168,7 +173,7 @@
         .fd = pipe_read.get(), .events = POLLIN, .revents = 0,
     };
 
-    rc = poll(&pfd, 1, remaining_ms.count());
+    rc = poll(&pfd, 1, remaining_ms);
     if (rc == -1) {
       if (errno == EINTR) {
         continue;

debuggerd/client/debuggerd_client_test.cpp

diff --git a/debuggerd/client/debuggerd_client_test.cpp b/debuggerd/client/debuggerd_client_test.cpp
index 86d0314..aff03e5 100644
--- a/debuggerd/client/debuggerd_client_test.cpp
+++ b/debuggerd/client/debuggerd_client_test.cpp
@@ -89,3 +89,23 @@
 
   EXPECT_EQ(1, found_end) << "\nOutput: \n" << result;
 }
+
+TEST(debuggerd_client, no_timeout) {
+  unique_fd pipe_read, pipe_write;
+  ASSERT_TRUE(Pipe(&pipe_read, &pipe_write));
+
+  pid_t forkpid = fork();
+  ASSERT_NE(-1, forkpid);
+  if (forkpid == 0) {
+    pipe_write.reset();
+    char dummy;
+    TEMP_FAILURE_RETRY(read(pipe_read.get(), &dummy, sizeof(dummy)));
+    exit(0);
+  }
+
+  pipe_read.reset();
+
+  unique_fd output_read, output_write;
+  ASSERT_TRUE(Pipe(&output_read, &output_write));
+  ASSERT_TRUE(debuggerd_trigger_dump(forkpid, std::move(output_write), kDebuggerdBacktrace, 0));
+}

debuggerd/include/debuggerd/client.h

diff --git a/debuggerd/include/debuggerd/client.h b/debuggerd/include/debuggerd/client.h
index 91f143b..01de57b 100644
--- a/debuggerd/include/debuggerd/client.h
+++ b/debuggerd/include/debuggerd/client.h
@@ -28,9 +28,9 @@
 };
 
 // Trigger a dump of specified process to output_fd.
-// output_fd is *not* consumed, timeouts <= 0 will wait forever.
+// output_fd is consumed, timeout of 0 will wait forever.
 bool debuggerd_trigger_dump(pid_t pid, android::base::unique_fd output_fd,
-                            enum DebuggerdDumpType dump_type, int timeout_ms);
+                            enum DebuggerdDumpType dump_type, unsigned int timeout_ms);
 
 int dump_backtrace_to_file(pid_t tid, int fd);
 int dump_backtrace_to_file_timeout(pid_t tid, int fd, int timeout_secs);