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
diff --git a/cmds/incidentd/src/Broadcaster.cpp b/cmds/incidentd/src/Broadcaster.cpp
index 39e5393..63464f2 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 @@
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 4ba31b4..b4021d1 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 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 @@
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 e773e74..218c1b2 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -551,7 +551,7 @@
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 aa376dd..ae640c6 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 @@
*/
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 @@
}
}
-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;
+ status_t err;
+
+ 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());
+ }
}
- *fd = fds[0];
- int writeFd = fds[1];
+ if (mEnvelope.has_metadata()) {
+ write_section(writeFd, FIELD_ID_INCIDENT_METADATA, mEnvelope.metadata());
+ }
- // 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);
+ if (err != NO_ERROR) {
+ ALOGW("Error writing incident report '%s' to dropbox: %s", getDataFileName().c_str(),
+ strerror(-err));
+ }
- err = filter_and_write_report(writeFd, dataFd, mEnvelope.privacy_policy(), args);
- close(writeFd);
-
- 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;
- }
- });
-
- // 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 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 @@
// ================================================================================
-//
+//
WorkDirectory::WorkDirectory()
:mDirectory("/data/misc/incidents"),
diff --git a/cmds/incidentd/src/WorkDirectory.h b/cmds/incidentd/src/WorkDirectory.h
index e344371..3c6a2f2 100644
--- a/cmds/incidentd/src/WorkDirectory.h
+++ b/cmds/incidentd/src/WorkDirectory.h
@@ -135,14 +135,14 @@
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 be2f24f..4e8ff71 100644
--- a/cmds/incidentd/src/proto_util.cpp
+++ b/cmds/incidentd/src/proto_util.cpp
@@ -33,11 +33,10 @@
// 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 @@
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 b9df6cb..2c0ab48 100644
--- a/cmds/incidentd/src/proto_util.h
+++ b/cmds/incidentd/src/proto_util.h
@@ -32,7 +32,7 @@
/**
* 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