Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 460b336d authored by Josh Gao's avatar Josh Gao
Browse files

tombstoned: fix a race between intercept and crash_dump. 

Previously, there was no way to detect when tombstoned processed an
intercept request packet, making it possible for a intercept request
followed by a crash_dump being processed in the wrong order.

Add a response to intercept registration, to eliminate this race.

Test: debuggerd_test
Change-Id: If38c6d14081ebc86ff1ed0edd7afaeafc40a8381
parent 807a4580

debuggerd/client/debuggerd_client.cpp

+29 −19
Original line number Diff line number Diff line
@@ -65,24 +65,25 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du 
  auto time_left = [timeout_ms, &end]() { return end - std::chrono::steady_clock::now(); };

  auto set_timeout = [timeout_ms, &time_left](int sockfd) {

    if (timeout_ms <= 0) {

      return true;

      return -1;

    }



    auto remaining = time_left();

    if (remaining < decltype(remaining)::zero()) {

      return false;

      LOG(ERROR) << "timeout expired";

      return -1;

    }

    struct timeval timeout;

    populate_timeval(&timeout, remaining);



    if (setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)) != 0) {

      return false;

      return -1;

    }

    if (setsockopt(sockfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)) != 0) {

      return false;

      return -1;

    }



    return true;

    return sockfd;

  };



  sockfd.reset(socket(AF_LOCAL, SOCK_SEQPACKET, 0));
@@ -91,12 +92,7 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du 
    return false;

  }



  if (!set_timeout(sockfd)) {

    PLOG(ERROR) << "libdebugger_client: failed to set timeout";

    return false;

  }



  if (socket_local_client_connect(sockfd.get(), kTombstonedInterceptSocketName,

  if (socket_local_client_connect(set_timeout(sockfd.get()), kTombstonedInterceptSocketName,

                                  ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET) == -1) {

    PLOG(ERROR) << "libdebuggerd_client: failed to connect to tombstoned";

    return false;
@@ -115,21 +111,35 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du 
    return false;

  }



  if (send_fd(sockfd.get(), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) {

  if (send_fd(set_timeout(sockfd), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) {

    PLOG(ERROR) << "libdebuggerd_client: failed to send output fd to tombstoned";

    return false;

  }



  bool backtrace = dump_type == kDebuggerdBacktrace;

  send_signal(pid, backtrace);

  // Check to make sure we've successfully registered.

  InterceptResponse response;

  ssize_t rc =

      TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC));

  if (rc == 0) {

    LOG(ERROR) << "libdebuggerd_client: failed to read response from tombstoned: timeout reached?";

    return false;

  } else if (rc != sizeof(response)) {

    LOG(ERROR)

        << "libdebuggerd_client: received packet of unexpected length from tombstoned: expected "

        << sizeof(response) << ", received " << rc;

    return false;

  }



  if (!set_timeout(sockfd)) {

    PLOG(ERROR) << "libdebuggerd_client: failed to set timeout";

  if (response.status != InterceptStatus::kRegistered) {

    LOG(ERROR) << "libdebuggerd_client: unexpected registration response: "

               << static_cast<int>(response.status);

    return false;

  }



  InterceptResponse response;

  ssize_t rc = TEMP_FAILURE_RETRY(recv(sockfd.get(), &response, sizeof(response), MSG_TRUNC));

  bool backtrace = dump_type == kDebuggerdBacktrace;

  send_signal(pid, backtrace);



  rc = TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC));

  if (rc == 0) {

    LOG(ERROR) << "libdebuggerd_client: failed to read response from tombstoned: timeout reached?";

    return false;
@@ -140,7 +150,7 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du 
    return false;

  }



  if (response.success != 1) {

  if (response.status != InterceptStatus::kStarted) {

    response.error_message[sizeof(response.error_message) - 1] = '\0';

    LOG(ERROR) << "libdebuggerd_client: tombstoned reported failure: " << response.error_message;

    return false;

debuggerd/debuggerd_test.cpp

+50 −33
Original line number Diff line number Diff line
@@ -77,6 +77,54 @@ constexpr char kWaitForGdbKey[] = "debug.debuggerd.wait_for_gdb"; 
    }                                                                           \

  } while (0)



static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, unique_fd* output_fd) {

  intercept_fd->reset(socket_local_client(kTombstonedInterceptSocketName,

                                          ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET));

  if (intercept_fd->get() == -1) {

    FAIL() << "failed to contact tombstoned: " << strerror(errno);

  }



  InterceptRequest req = {.pid = target_pid};



  unique_fd output_pipe_write;

  if (!Pipe(output_fd, &output_pipe_write)) {

    FAIL() << "failed to create output pipe: " << strerror(errno);

  }



  std::string pipe_size_str;

  int pipe_buffer_size;

  if (!android::base::ReadFileToString("/proc/sys/fs/pipe-max-size", &pipe_size_str)) {

    FAIL() << "failed to read /proc/sys/fs/pipe-max-size: " << strerror(errno);

  }



  pipe_size_str = android::base::Trim(pipe_size_str);



  if (!android::base::ParseInt(pipe_size_str.c_str(), &pipe_buffer_size, 0)) {

    FAIL() << "failed to parse pipe max size";

  }



  if (fcntl(output_fd->get(), F_SETPIPE_SZ, pipe_buffer_size) != pipe_buffer_size) {

    FAIL() << "failed to set pipe size: " << strerror(errno);

  }



  if (send_fd(intercept_fd->get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) {

    FAIL() << "failed to send output fd to tombstoned: " << strerror(errno);

  }



  InterceptResponse response;

  ssize_t rc = TEMP_FAILURE_RETRY(read(intercept_fd->get(), &response, sizeof(response)));

  if (rc == -1) {

    FAIL() << "failed to read response from tombstoned: " << strerror(errno);

  } else if (rc == 0) {

    FAIL() << "failed to read response from tombstoned (EOF)";

  } else if (rc != sizeof(response)) {

    FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(response)

           << ", received " << rc;

  }



  ASSERT_EQ(InterceptStatus::kRegistered, response.status);

}



class CrasherTest : public ::testing::Test {

 public:

  pid_t crasher_pid = -1;
@@ -118,38 +166,7 @@ void CrasherTest::StartIntercept(unique_fd* output_fd) { 
    FAIL() << "crasher hasn't been started";

  }



  intercept_fd.reset(socket_local_client(kTombstonedInterceptSocketName,

                                         ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET));

  if (intercept_fd == -1) {

    FAIL() << "failed to contact tombstoned: " << strerror(errno);

  }



  InterceptRequest req = {.pid = crasher_pid };



  unique_fd output_pipe_write;

  if (!Pipe(output_fd, &output_pipe_write)) {

    FAIL() << "failed to create output pipe: " << strerror(errno);

  }



  std::string pipe_size_str;

  int pipe_buffer_size;

  if (!android::base::ReadFileToString("/proc/sys/fs/pipe-max-size", &pipe_size_str)) {

    FAIL() << "failed to read /proc/sys/fs/pipe-max-size: " << strerror(errno);

  }



  pipe_size_str = android::base::Trim(pipe_size_str);



  if (!android::base::ParseInt(pipe_size_str.c_str(), &pipe_buffer_size, 0)) {

    FAIL() << "failed to parse pipe max size";

  }



  if (fcntl(output_fd->get(), F_SETPIPE_SZ, pipe_buffer_size) != pipe_buffer_size) {

    FAIL() << "failed to set pipe size: " << strerror(errno);

  }



  if (send_fd(intercept_fd.get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) {

    FAIL() << "failed to send output fd to tombstoned: " << strerror(errno);

  }

  tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd);

}



void CrasherTest::FinishIntercept(int* result) {
@@ -165,7 +182,7 @@ void CrasherTest::FinishIntercept(int* result) { 
    FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(response)

           << ", received " << rc;

  } else {

    *result = response.success;

    *result = response.status == InterceptStatus::kStarted ? 1 : 0;

  }

}

debuggerd/include/debuggerd/protocol.h

+7 −1
Original line number Diff line number Diff line
@@ -56,8 +56,14 @@ struct InterceptRequest { 
  int32_t pid;

};



enum class InterceptStatus : uint8_t {

  kFailed,

  kStarted,

  kRegistered,

};



// Sent either immediately upon failure, or when the intercept has been used.

struct InterceptResponse {

  uint8_t success;          // 0 or 1

  InterceptStatus status;

  char error_message[127];  // always null-terminated

};

debuggerd/tombstoned/intercept_manager.cpp

+13 −2
Original line number Diff line number Diff line
@@ -105,6 +105,7 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { 
    // We trust the other side, so only do minimal validity checking.

    if (intercept_request.pid <= 0 || intercept_request.pid > std::numeric_limits<pid_t>::max()) {

      InterceptResponse response = {};

      response.status = InterceptStatus::kFailed;

      snprintf(response.error_message, sizeof(response.error_message), "invalid pid %" PRId32,

               intercept_request.pid);

      TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response)));
@@ -113,9 +114,10 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { 


    intercept->intercept_pid = intercept_request.pid;



    // Register the intercept with the InterceptManager.

    // Check if it's already registered.

    if (intercept_manager->intercepts.count(intercept_request.pid) > 0) {

      InterceptResponse response = {};

      response.status = InterceptStatus::kFailed;

      snprintf(response.error_message, sizeof(response.error_message),

               "pid %" PRId32 " already intercepted", intercept_request.pid);

      TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response)));
@@ -123,6 +125,15 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { 
      goto fail;

    }



    // Let the other side know that the intercept has been registered, now that we know we can't

    // fail. tombstoned is single threaded, so this isn't racy.

    InterceptResponse response = {};

    response.status = InterceptStatus::kRegistered;

    if (TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response))) == -1) {

      PLOG(WARNING) << "failed to notify interceptor of registration";

      goto fail;

    }



    intercept->output_fd = std::move(rcv_fd);

    intercept_manager->intercepts[intercept_request.pid] = std::unique_ptr<Intercept>(intercept);

    intercept->registered = true;
@@ -174,7 +185,7 @@ bool InterceptManager::GetIntercept(pid_t pid, android::base::unique_fd* out_fd) 


  LOG(INFO) << "found intercept fd " << intercept->output_fd.get() << " for pid " << pid;

  InterceptResponse response = {};

  response.success = 1;

  response.status = InterceptStatus::kStarted;

  TEMP_FAILURE_RETRY(write(intercept->sockfd, &response, sizeof(response)));

  *out_fd = std::move(intercept->output_fd);