Add metadata and headers to incident reports.

+ Remove the spawned thread inside the ReportFile for filter_and_write_report
  because it leads to accessing freed memory

  Instead, let the caller of ReportFile::startFileteringData create the thread.
  ReportFile class shouldn't care about whether it's writing to a pipe for IPC
  or regular file.

+ Add uri building in incidentd

+ Add metadata and headers to incident reports

Test: existing passed tests in incidentd_test still pass.
      Manually tested with statsd

Change-Id: I5fef900d31f5d181275814f1e1c8c98443f201a7

7 files changed, 68 insertions, 63 deletions

diff --git a/cmds/incidentd/src/Broadcaster.cpp b/cmds/incidentd/src/Broadcaster.cpp
index 39e5393e1f81..63464f2d58c5 100644
--- a/cmds/incidentd/src/Broadcaster.cpp
+++ b/cmds/incidentd/src/Broadcaster.cpp
@@ -22,6 +22,7 @@
 
 #include <android/os/DropBoxManager.h>
 #include <binder/IServiceManager.h>
+#include <thread>
 
 namespace android {
 namespace os {
@@ -391,13 +392,20 @@ status_t Broadcaster::send_to_dropbox(const sp<ReportFile>& file,
         return NO_ERROR;
     }
 
-    // Start a thread to write the data to dropbox.
-    int readFd = -1;
-    err = file->startFilteringData(&readFd, args);
-    if (err != NO_ERROR) {
-        return err;
+    int fds[2];
+    if (pipe(fds) != 0) {
+        ALOGW("Error opening pipe to filter incident report: %s", file->getDataFileName().c_str());
+        return NO_ERROR;
     }
 
+    int readFd = fds[0];
+    int writeFd = fds[1];
+
+    // spawn a thread to write the data. Release the writeFd ownership to the thread.
+    thread th([file, writeFd, args]() { file->startFilteringData(writeFd, args); });
+
+    th.detach();
+
     // Takes ownership of readFd.
     Status status = dropbox->addFile(String16("incident"), readFd, 0);
     if (!status.isOk()) {
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index 4ba31b45e81c..b4021d1b2f76 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -32,6 +32,7 @@
 #include <log/log.h>
 #include <private/android_filesystem_config.h>
 #include <utils/Looper.h>
+#include <thread>
 
 #include <unistd.h>
 
@@ -117,7 +118,8 @@ static Status checkIncidentPermissions(const IncidentReportArgs& args) {
 }
 
 static string build_uri(const string& pkg, const string& cls, const string& id) {
-    return "build_uri not implemented " + pkg + "/" + cls + "/" + id;
+    return "content://android.os.IncidentManager/pending?pkg="
+        + pkg + "&receiver=" + cls + "&r=" + id;
 }
 
 // ================================================================================
@@ -358,17 +360,21 @@ Status IncidentService::getIncidentReport(const String16& pkg16, const String16&
     IncidentReportArgs args;
     sp<ReportFile> file = mWorkDirectory->getReport(pkg, cls, id, &args);
     if (file != nullptr) {
-        int fd;
-        err = file->startFilteringData(&fd, args);
-        if (err != 0) {
-            ALOGW("Error reading data file that we think should exist: %s",
-                    file->getDataFileName().c_str());
+        // Create pipe
+        int fds[2];
+        if (pipe(fds) != 0) {
+            ALOGW("Error opening pipe to filter incident report: %s",
+                  file->getDataFileName().c_str());
             return Status::ok();
         }
-
         result->setTimestampNs(file->getTimestampNs());
         result->setPrivacyPolicy(file->getEnvelope().privacy_policy());
-        result->takeFileDescriptor(fd);
+        result->takeFileDescriptor(fds[0]);
+        int writeFd = fds[1];
+        // spawn a thread to write the data. Release the writeFd ownership to the thread.
+        thread th([file, writeFd, args]() { file->startFilteringData(writeFd, args); });
+
+        th.detach();
     }
 
     return Status::ok();
diff --git a/cmds/incidentd/src/Reporter.cpp b/cmds/incidentd/src/Reporter.cpp
index e773e74bbf15..218c1b27fdcc 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -551,7 +551,7 @@ void Reporter::runReport(size_t* reportByteSize) {
              buf++) {
             // If there was an error now, there will be an error later and we will remove
             // it from the list then.
-            write_header_section(request->getFd(), *buf);
+            write_header_section(request->getFd(), buf->data(), buf->size());
         }
     });
 
diff --git a/cmds/incidentd/src/WorkDirectory.cpp b/cmds/incidentd/src/WorkDirectory.cpp
index aa376ddee082..ae640c635803 100644
--- a/cmds/incidentd/src/WorkDirectory.cpp
+++ b/cmds/incidentd/src/WorkDirectory.cpp
@@ -18,6 +18,7 @@
 
 #include "WorkDirectory.h"
 
+#include "proto_util.h"
 #include "PrivacyFilter.h"
 
 #include <google/protobuf/io/zero_copy_stream_impl.h>
@@ -64,6 +65,9 @@ static const string EXTENSION_DATA(".data");
  */
 const ComponentName DROPBOX_SENTINEL("android", "DROPBOX");
 
+/** metadata field id in IncidentProto */
+const int FIELD_ID_INCIDENT_METADATA = 2;
+
 /**
  * Read a protobuf from disk into the message.
  */
@@ -386,65 +390,53 @@ void ReportFile::closeDataFile() {
     }
 }
 
-status_t ReportFile::startFilteringData(int* fd, const IncidentReportArgs& args) {
+status_t ReportFile::startFilteringData(int writeFd, const IncidentReportArgs& args) {
     // Open data file.
     int dataFd = open(mDataFileName.c_str(), O_RDONLY | O_CLOEXEC);
     if (dataFd < 0) {
+        ALOGW("Error opening incident report '%s' %s", getDataFileName().c_str(), strerror(-errno));
+        close(writeFd);
         return -errno;
     }
 
     // Check that the size on disk is what we thought we wrote.
     struct stat st;
     if (fstat(dataFd, &st) != 0) {
+        ALOGW("Error running fstat incident report '%s' %s", getDataFileName().c_str(),
+              strerror(-errno));
+        close(writeFd);
         return -errno;
     }
     if (st.st_size != mEnvelope.data_file_size()) {
         ALOGW("File size mismatch. Envelope says %" PRIi64 " bytes but data file is %" PRIi64
-                " bytes: %s", (int64_t)mEnvelope.data_file_size(), st.st_size,
-                mDataFileName.c_str());
+              " bytes: %s",
+              (int64_t)mEnvelope.data_file_size(), st.st_size, mDataFileName.c_str());
         ALOGW("Removing incident report");
         mWorkDirectory->remove(this);
+        close(writeFd);
         return BAD_VALUE;
     }
 
-    // Create pipe
-    int fds[2];
-    if (pipe(fds) != 0) {
-        ALOGW("Error opening pipe to filter incident report: %s", getDataFileName().c_str());
-        return -errno;
-    }
-
-    *fd = fds[0];
-    int writeFd = fds[1];
-
-    // Spawn off a thread to do the filtering and writing
-    thread th([this, dataFd, writeFd, args]() {
-        ALOGD("worker thread started dataFd=%d writeFd=%d", dataFd, writeFd);
-        status_t err;
-
-        err = filter_and_write_report(writeFd, dataFd, mEnvelope.privacy_policy(), args);
-        close(writeFd);
+    status_t err;
 
-        if (err != NO_ERROR) {
-            ALOGW("Error writing incident report '%s' to dropbox: %s", getDataFileName().c_str(),
-                    strerror(-err));
-            // If there's an error here, there will also be an error returned from
-            // addFile, so we'll use that error to reschedule the send_to_dropbox.
-            // If the file is corrupted, we will put some logs in logcat, but won't
-            // actually return an error.
-            return;
+    for (const auto& report : mEnvelope.report()) {
+        for (const auto& header : report.header()) {
+           write_header_section(writeFd,
+               reinterpret_cast<const uint8_t*>(header.c_str()), header.size());
         }
-    });
+    }
 
-    // Better would be to join this thread after write is back, but there is no
-    // timeout parameter for that, which means we can't clean up if system server
-    // is stuck. Better is to leak the thread, which will eventually clean itself
-    // up after system server eventually dies, which it probably will.
-    th.detach();
+    if (mEnvelope.has_metadata()) {
+        write_section(writeFd, FIELD_ID_INCIDENT_METADATA, mEnvelope.metadata());
+    }
+
+    err = filter_and_write_report(writeFd, dataFd, mEnvelope.privacy_policy(), args);
+    if (err != NO_ERROR) {
+        ALOGW("Error writing incident report '%s' to dropbox: %s", getDataFileName().c_str(),
+                strerror(-err));
+    }
 
-    // If the thread fails to start, we should return an error, but the thread
-    // class doesn't give us a good way to determine that.  Just pretend everything
-    // is ok.
+    close(writeFd);
     return NO_ERROR;
 }
 
@@ -501,7 +493,7 @@ status_t ReportFile::load_envelope_impl(bool cleanup) {
 
 
 // ================================================================================
-// 
+//
 
 WorkDirectory::WorkDirectory()
         :mDirectory("/data/misc/incidents"),
diff --git a/cmds/incidentd/src/WorkDirectory.h b/cmds/incidentd/src/WorkDirectory.h
index e344371c3682..3c6a2f2b9617 100644
--- a/cmds/incidentd/src/WorkDirectory.h
+++ b/cmds/incidentd/src/WorkDirectory.h
@@ -135,14 +135,14 @@ public:
     void closeDataFile();
 
     /**
-     * Spawn a thread to start writing and filtering data to a pipe, the read end of which
-     * will be returned in fd.  This thread will be detached and run until somebody finishes
-     * reading from the fd or closes it.  If there is an error, returns it and you will not
-     * get an fd.
+     * Use the privacy and section configuration from the args parameter to filter data, write
+     * to [writeFd] and take the ownership of [writeFd].
      *
-     * Use the privacy and section configuraiton from the args parameter.
+     * Note: this call is blocking. When the writeFd is a pipe fd for IPC, caller should make sure
+     * it's called on a separate thread so that reader can start to read without waiting for writer
+     * to finish writing (which may not happen due to pipe buffer overflow).
      */
-    status_t startFilteringData(int* fd, const IncidentReportArgs& args);
+    status_t startFilteringData(int writeFd, const IncidentReportArgs& args);
 
     /**
      * Get the name of the data file on disk.
diff --git a/cmds/incidentd/src/proto_util.cpp b/cmds/incidentd/src/proto_util.cpp
index be2f24f97d02..4e8ff71b3777 100644
--- a/cmds/incidentd/src/proto_util.cpp
+++ b/cmds/incidentd/src/proto_util.cpp
@@ -33,11 +33,10 @@ using google::protobuf::io::FileOutputStream;
 // special section ids
 const int FIELD_ID_INCIDENT_HEADER = 1;
 
-status_t write_header_section(int fd, const vector<uint8_t>& buf) {
+status_t write_header_section(int fd, const uint8_t* buf, size_t bufSize) {
     status_t err;
-    const size_t bufSize = buf.size();
 
-    if (buf.empty()) {
+    if (bufSize == 0) {
         return NO_ERROR;
     }
 
@@ -46,7 +45,7 @@ status_t write_header_section(int fd, const vector<uint8_t>& buf) {
         return err;
     }
 
-    err = WriteFully(fd, (uint8_t const*)buf.data(), bufSize);
+    err = WriteFully(fd, buf, bufSize);
     if (err != NO_ERROR) {
         return err;
     }
diff --git a/cmds/incidentd/src/proto_util.h b/cmds/incidentd/src/proto_util.h
index b9df6cbf296d..2c0ab4893ba5 100644
--- a/cmds/incidentd/src/proto_util.h
+++ b/cmds/incidentd/src/proto_util.h
@@ -32,7 +32,7 @@ using google::protobuf::MessageLite;
 /**
  * Write the IncidentHeaderProto section
  */
-status_t write_header_section(int fd, const vector<uint8_t>& buf);
+status_t write_header_section(int fd, const uint8_t* buf, size_t bufSize);
 
 /**
  * Write the prologue for a section in the incident report