From 0f0471623e91c202fb7381a050cc331572fb439f Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Tue, 5 Sep 2017 13:44:22 -0700 Subject: [PATCH 1/2] Implement Pii Stripper Part 3 The incident request args sets privacy spec. Strip action is optimized to run once for each type of spec and ready for flush multiple times. Incident command is updated to take -p option to specify privacy spec. Bug: 64687253 Test: unit tests written, manually run incident command to test as well Change-Id: I6753df117f76dc1a5f4d2152baa3fbbf56b490e4 --- cmds/incident/main.cpp | 21 ++- cmds/incidentd/src/EncodedBuffer.cpp | 4 +- cmds/incidentd/src/FdBuffer.cpp | 59 ++++++++ cmds/incidentd/src/FdBuffer.h | 29 ++-- cmds/incidentd/src/Privacy.cpp | 11 ++ cmds/incidentd/src/Privacy.h | 3 + cmds/incidentd/src/Section.cpp | 50 ++++--- cmds/incidentd/tests/Reporter_test.cpp | 3 +- cmds/incidentd/tests/Section_test.cpp | 130 ++++++++++++++++++ core/java/android/os/IncidentReportArgs.java | 16 ++- .../include/android/os/IncidentReportArgs.h | 6 +- libs/incident/src/IncidentReportArgs.cpp | 24 +++- 12 files changed, 312 insertions(+), 44 deletions(-) diff --git a/cmds/incident/main.cpp b/cmds/incident/main.cpp index 47f1db89e1cbe..519852dbe88b8 100644 --- a/cmds/incident/main.cpp +++ b/cmds/incident/main.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -143,6 +144,16 @@ find_section(const char* name) return NULL; } +// ================================================================================ +static int +get_dest(const char* arg) +{ + if (strcmp(arg, "LOCAL") == 0) return 0; + if (strcmp(arg, "EXPLICIT") == 0) return 1; + if (strcmp(arg, "AUTOMATIC") == 0) return 2; + return -1; // return the default value +} + // ================================================================================ static void usage(FILE* out) @@ -155,6 +166,7 @@ usage(FILE* out) fprintf(out, " -b (default) print the report to stdout (in proto format)\n"); fprintf(out, " -d send the report into dropbox\n"); fprintf(out, " -l list available sections\n"); + fprintf(out, " -p privacy spec, LOCAL, EXPLICIT or AUTOMATIC\n"); fprintf(out, "\n"); fprintf(out, " SECTION the field numbers of the incident report fields to include\n"); fprintf(out, "\n"); @@ -166,10 +178,11 @@ main(int argc, char** argv) Status status; IncidentReportArgs args; enum { DEST_DROPBOX, DEST_STDOUT } destination = DEST_STDOUT; + int dest = -1; // default // Parse the args int opt; - while ((opt = getopt(argc, argv, "bhdl")) != -1) { + while ((opt = getopt(argc, argv, "bhdlp:")) != -1) { switch (opt) { case 'h': usage(stdout); @@ -183,6 +196,9 @@ main(int argc, char** argv) case 'd': destination = DEST_DROPBOX; break; + case 'p': + dest = get_dest(optarg); + break; default: usage(stderr); return 1; @@ -210,8 +226,7 @@ main(int argc, char** argv) } } } - - + args.setDest(dest); // Start the thread pool. sp ps(ProcessState::self()); diff --git a/cmds/incidentd/src/EncodedBuffer.cpp b/cmds/incidentd/src/EncodedBuffer.cpp index d1872f96b9df4..3d20548f18b7e 100644 --- a/cmds/incidentd/src/EncodedBuffer.cpp +++ b/cmds/incidentd/src/EncodedBuffer.cpp @@ -70,7 +70,6 @@ write_field_or_skip(FdBuffer::iterator &iterator, vector &buf, uint8_t if (skip) { iterator += bytesToWrite; } else { - buf.reserve(bytesToWrite); for (size_t i=0; i mFdBuffer.size(); +} diff --git a/cmds/incidentd/src/FdBuffer.h b/cmds/incidentd/src/FdBuffer.h index 4c4823e5a4d88..dfe39c62de426 100644 --- a/cmds/incidentd/src/FdBuffer.h +++ b/cmds/incidentd/src/FdBuffer.h @@ -87,32 +87,29 @@ public: friend class iterator; class iterator : public std::iterator { public: - iterator(const FdBuffer& buffer, ssize_t index, ssize_t offset) - : mFdBuffer(buffer), mIndex(index), mOffset(offset) {} - iterator& operator=(iterator& other) const { return other; } - iterator& operator+(size_t offset); // this is implemented in .cpp - iterator& operator+=(size_t offset) { return *this + offset; } - iterator& operator++() { return *this + 1; } - iterator operator++(int) { return *this + 1; } - bool operator==(iterator other) const { - return mIndex == other.mIndex && mOffset == other.mOffset; - } - bool operator!=(iterator other) const { return !(*this == other); } - int operator-(iterator other) const { return (int)bytesRead() - (int)other.bytesRead(); } - reference operator*() const { return mFdBuffer.mBuffers[mIndex][mOffset]; } + iterator(const FdBuffer& buffer, ssize_t index, ssize_t offset); + iterator& operator=(iterator& other) const; + iterator& operator+(size_t offset); + iterator& operator+=(size_t offset); + iterator& operator++(); + iterator operator++(int); + bool operator==(iterator other) const; + bool operator!=(iterator other) const; + int operator-(iterator other) const; + reference operator*() const; // return the snapshot of the current iterator - iterator snapshot() const { return iterator(mFdBuffer, mIndex, mOffset); } + iterator snapshot() const; // how many bytes are read size_t bytesRead() const; // random access could make the iterator out of bound - bool outOfBound() const { return bytesRead() > mFdBuffer.size(); } + bool outOfBound() const; private: const FdBuffer& mFdBuffer; size_t mIndex; size_t mOffset; }; - iterator begin() const { return iterator(*this, 0, 0); } + iterator begin() const; iterator end() const; private: diff --git a/cmds/incidentd/src/Privacy.cpp b/cmds/incidentd/src/Privacy.cpp index a790efa2f6113..dbab5480e6982 100644 --- a/cmds/incidentd/src/Privacy.cpp +++ b/cmds/incidentd/src/Privacy.cpp @@ -87,6 +87,12 @@ static bool allowDest(const uint8_t dest, const uint8_t policy) } } +bool +PrivacySpec::operator<(const PrivacySpec& other) const +{ + return dest < other.dest; +} + bool PrivacySpec::CheckPremission(const Privacy* privacy) const { @@ -97,4 +103,9 @@ PrivacySpec::CheckPremission(const Privacy* privacy) const bool PrivacySpec::RequireAll() const { return dest == DEST_LOCAL; } +PrivacySpec new_spec_from_args(int dest) { + if (dest < 0) return PrivacySpec(); + return PrivacySpec(dest); +} + PrivacySpec get_default_dropbox_spec() { return PrivacySpec(DEST_AUTOMATIC); } \ No newline at end of file diff --git a/cmds/incidentd/src/Privacy.h b/cmds/incidentd/src/Privacy.h index 53b0325b15f46..c56ba9b8e3fe9 100644 --- a/cmds/incidentd/src/Privacy.h +++ b/cmds/incidentd/src/Privacy.h @@ -58,10 +58,13 @@ public: PrivacySpec() : dest(DEST_DEFAULT_VALUE) {} PrivacySpec(uint8_t dest) : dest(dest) {} + bool operator<(const PrivacySpec& other) const; + bool CheckPremission(const Privacy* privacy) const; bool RequireAll() const; }; +PrivacySpec new_spec_from_args(int dest); PrivacySpec get_default_dropbox_spec(); #endif // PRIVACY_H diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp index a04804c8c7702..08959263838d2 100644 --- a/cmds/incidentd/src/Section.cpp +++ b/cmds/incidentd/src/Section.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -109,20 +110,16 @@ GetPrivacyOfSection(int id) } static status_t -WriteToRequest(const int id, const int fd, EncodedBuffer& buffer, const PrivacySpec& spec) +WriteToRequest(const int id, const int fd, EncodedBuffer& buffer) { - if (fd < 0) return EBADF; + if (buffer.size() == 0) return NO_ERROR; + status_t err = NO_ERROR; uint8_t buf[20]; - - buffer.clear(); // clear before strip - err = buffer.strip(spec); // TODO: don't have to strip again if the spec is the same. - if (err != NO_ERROR || buffer.size() == 0) return err; uint8_t *p = write_length_delimited_tag_header(buf, id, buffer.size()); err = write_all(fd, buf, p-buf); if (err == NO_ERROR) { err = buffer.flush(fd); - ALOGD("Section %d flushed %zu bytes to fd %d with spec %d", id, buffer.size(), fd, spec.dest); } return err; } @@ -130,25 +127,46 @@ WriteToRequest(const int id, const int fd, EncodedBuffer& buffer, const PrivacyS static status_t WriteToReportRequests(const int id, const FdBuffer& buffer, ReportRequestSet* requests) { - status_t err = EBADF; + status_t err = -EBADF; EncodedBuffer encodedBuffer(buffer, GetPrivacyOfSection(id)); int writeable = 0; - // The streaming ones + // The streaming ones, group requests by spec in order to save unnecessary strip operations + map>> requestsBySpec; for (ReportRequestSet::iterator it = requests->begin(); it != requests->end(); it++) { sp request = *it; - PrivacySpec spec; // TODO: this should be derived from each request. - err = WriteToRequest(id, request->fd, encodedBuffer, spec); - if (err != NO_ERROR) { - request->err = err; - } else { - writeable++; + if (!request->args.containsSection(id) || request->fd < 0 || request->err != NO_ERROR) { + continue; // skip invalid request } + PrivacySpec spec = new_spec_from_args(request->args.dest()); + requestsBySpec[spec].push_back(request); + } + + for (map>>::iterator mit = requestsBySpec.begin(); mit != requestsBySpec.end(); mit++) { + PrivacySpec spec = mit->first; + err = encodedBuffer.strip(spec); + if (err != NO_ERROR) return err; // it means the encodedBuffer data is corrupted. + if (encodedBuffer.size() == 0) continue; + + for (vector>::iterator it = mit->second.begin(); it != mit->second.end(); it++) { + sp request = *it; + err = WriteToRequest(id, request->fd, encodedBuffer); + if (err != NO_ERROR) { + request->err = err; + } else { + writeable++; + } + ALOGD("Section %d flushed %zu bytes to fd %d with spec %d", id, encodedBuffer.size(), request->fd, spec.dest); + } + encodedBuffer.clear(); } // The dropbox file if (requests->mainFd() >= 0) { - err = WriteToRequest(id, requests->mainFd(), encodedBuffer, get_default_dropbox_spec()); + err = encodedBuffer.strip(get_default_dropbox_spec()); + if (err != NO_ERROR) return err; // the buffer data is corrupted. + + err = WriteToRequest(id, requests->mainFd(), encodedBuffer); if (err != NO_ERROR) { requests->setMainFd(-1); } else { diff --git a/cmds/incidentd/tests/Reporter_test.cpp b/cmds/incidentd/tests/Reporter_test.cpp index 3c1b44b6a515b..5d074bcb0e4c2 100644 --- a/cmds/incidentd/tests/Reporter_test.cpp +++ b/cmds/incidentd/tests/Reporter_test.cpp @@ -76,8 +76,7 @@ public: }; protected: - IBinder* onAsBinder() override { return nullptr; }; - + virtual IBinder* onAsBinder() override { return nullptr; }; }; class ReporterTest : public Test { diff --git a/cmds/incidentd/tests/Section_test.cpp b/cmds/incidentd/tests/Section_test.cpp index ab0f054982320..f2aee8fa2bb07 100644 --- a/cmds/incidentd/tests/Section_test.cpp +++ b/cmds/incidentd/tests/Section_test.cpp @@ -33,11 +33,27 @@ const string STRING_FIELD_2 = "\x12\vwhatthefuck"; const string FIX64_FIELD_3 = "\x19\xff\xff\xff\xff\xff\xff\xff\xff"; // -1 using namespace android::base; +using namespace android::binder; using namespace std; using ::testing::StrEq; using ::testing::internal::CaptureStdout; using ::testing::internal::GetCapturedStdout; +class SimpleListener : public IIncidentReportStatusListener +{ +public: + SimpleListener() {}; + virtual ~SimpleListener() {}; + + virtual Status onReportStarted() { return Status::ok(); }; + virtual Status onReportSectionStatus(int /*section*/, int /*status*/) { return Status::ok(); }; + virtual Status onReportFinished() { return Status::ok(); }; + virtual Status onReportFailed() { return Status::ok(); }; + +protected: + virtual IBinder* onAsBinder() override { return nullptr; }; +}; + // NOTICE: this test requires /system/bin/incident_helper is installed. TEST(SectionTest, FileSection) { TemporaryFile tf; @@ -126,4 +142,118 @@ TEST(SectionTest, TestFilterPiiTaggedFields) { CaptureStdout(); ASSERT_EQ(NO_ERROR, fs.Execute(&requests)); EXPECT_THAT(GetCapturedStdout(), StrEq("\x02\r" + STRING_FIELD_2)); +} + +TEST(SectionTest, TestBadFdRequest) { + TemporaryFile input; + FileSection fs(NOOP_PARSER, input.path); + ReportRequestSet requests; + ASSERT_TRUE(WriteStringToFile(VARINT_FIELD_1 + STRING_FIELD_2 + FIX64_FIELD_3, input.path, false)); + + IncidentReportArgs args; + args.setAll(true); + args.setDest(0); + sp badFdRequest = new ReportRequest(args, new SimpleListener(), 1234567); + requests.add(badFdRequest); + requests.setMainFd(STDOUT_FILENO); + + CaptureStdout(); + ASSERT_EQ(NO_ERROR, fs.Execute(&requests)); + EXPECT_THAT(GetCapturedStdout(), StrEq("\x02\r" + STRING_FIELD_2)); + EXPECT_EQ(badFdRequest->err, -EBADF); +} + +TEST(SectionTest, TestBadRequests) { + TemporaryFile input; + FileSection fs(NOOP_PARSER, input.path); + ReportRequestSet requests; + ASSERT_TRUE(WriteStringToFile(VARINT_FIELD_1 + STRING_FIELD_2 + FIX64_FIELD_3, input.path, false)); + + IncidentReportArgs args; + args.setAll(true); + args.setDest(0); + requests.add(new ReportRequest(args, new SimpleListener(), -1)); + EXPECT_EQ(fs.Execute(&requests), -EBADF); +} + +TEST(SectionTest, TestMultipleRequests) { + TemporaryFile input, output1, output2, output3; + FileSection fs(NOOP_PARSER, input.path); + ReportRequestSet requests; + + ASSERT_TRUE(input.fd != -1); + ASSERT_TRUE(output1.fd != -1); + ASSERT_TRUE(output2.fd != -1); + ASSERT_TRUE(output3.fd != -1); + ASSERT_TRUE(WriteStringToFile(VARINT_FIELD_1 + STRING_FIELD_2 + FIX64_FIELD_3, input.path, false)); + + IncidentReportArgs args1, args2, args3; + args1.setAll(true); + args1.setDest(0); // LOCAL + args2.setAll(true); // default to explicit + sp l = new SimpleListener(); + requests.add(new ReportRequest(args1, l, output1.fd)); + requests.add(new ReportRequest(args2, l, output2.fd)); + requests.add(new ReportRequest(args3, l, output3.fd)); + requests.setMainFd(STDOUT_FILENO); + + CaptureStdout(); + ASSERT_EQ(NO_ERROR, fs.Execute(&requests)); + EXPECT_THAT(GetCapturedStdout(), StrEq("\x02\r" + STRING_FIELD_2)); + + string content, expect; + expect = VARINT_FIELD_1 + STRING_FIELD_2 + FIX64_FIELD_3; + char c = (char) expect.size(); + EXPECT_TRUE(ReadFileToString(output1.path, &content)); + EXPECT_THAT(content, StrEq(string("\x02") + c + expect)); + + expect = STRING_FIELD_2 + FIX64_FIELD_3; + c = (char) expect.size(); + EXPECT_TRUE(ReadFileToString(output2.path, &content)); + EXPECT_THAT(content, StrEq(string("\x02") + c + expect)); + + // because args3 doesn't set section, so it should receive nothing + EXPECT_TRUE(ReadFileToString(output3.path, &content)); + EXPECT_THAT(content, StrEq("")); +} + +TEST(SectionTest, TestMultipleRequestsBySpec) { + TemporaryFile input, output1, output2, output3; + FileSection fs(NOOP_PARSER, input.path); + ReportRequestSet requests; + + ASSERT_TRUE(input.fd != -1); + ASSERT_TRUE(output1.fd != -1); + ASSERT_TRUE(output2.fd != -1); + ASSERT_TRUE(output3.fd != -1); + + ASSERT_TRUE(WriteStringToFile(VARINT_FIELD_1 + STRING_FIELD_2 + FIX64_FIELD_3, input.path, false)); + + IncidentReportArgs args1, args2, args3, args4; + args1.setAll(true); + args2.setAll(true); + args4.setAll(true); + sp l = new SimpleListener(); + requests.add(new ReportRequest(args1, l, output1.fd)); + requests.add(new ReportRequest(args2, l, output2.fd)); + requests.add(new ReportRequest(args3, l, output3.fd)); + requests.setMainFd(STDOUT_FILENO); + + CaptureStdout(); + ASSERT_EQ(NO_ERROR, fs.Execute(&requests)); + EXPECT_THAT(GetCapturedStdout(), StrEq("\x02\r" + STRING_FIELD_2)); + + string content, expect; + expect = STRING_FIELD_2 + FIX64_FIELD_3; + char c = (char) expect.size(); + + // output1 and output2 are the same + EXPECT_TRUE(ReadFileToString(output1.path, &content)); + EXPECT_THAT(content, StrEq(string("\x02") + c + expect)); + EXPECT_TRUE(ReadFileToString(output2.path, &content)); + EXPECT_THAT(content, StrEq(string("\x02") + c + expect)); + + // because args3 doesn't set section, so it should receive nothing + EXPECT_TRUE(ReadFileToString(output3.path, &content)); + EXPECT_THAT(content, StrEq("")); } \ No newline at end of file diff --git a/core/java/android/os/IncidentReportArgs.java b/core/java/android/os/IncidentReportArgs.java index abb3161713097..fd0ebcfea080a 100644 --- a/core/java/android/os/IncidentReportArgs.java +++ b/core/java/android/os/IncidentReportArgs.java @@ -35,6 +35,7 @@ public final class IncidentReportArgs implements Parcelable { private final IntArray mSections = new IntArray(); private final ArrayList mHeaders = new ArrayList(); private boolean mAll; + private int mDest; /** * Construct an incident report args with no fields. @@ -69,6 +70,8 @@ public final class IncidentReportArgs implements Parcelable { for (int i=0; i CREATOR @@ -118,7 +123,8 @@ public final class IncidentReportArgs implements Parcelable { } sb.append(", "); sb.append(mHeaders.size()); - sb.append(" headers)"); + sb.append(" headers), "); + sb.append("Dest enum value: ").append(mDest); return sb.toString(); } @@ -132,6 +138,14 @@ public final class IncidentReportArgs implements Parcelable { } } + /** + * Set this incident report privacy policy spec. + * @hide + */ + public void setPrivacyPolicy(int dest) { + mDest = dest; + } + /** * Add this section to the incident report. Skip if the input is smaller than 2 since section * id are only valid for positive integer as Protobuf field id. Here 1 is reserved for Header. diff --git a/libs/incident/include/android/os/IncidentReportArgs.h b/libs/incident/include/android/os/IncidentReportArgs.h index 956ef6c39b993..da8098970962e 100644 --- a/libs/incident/include/android/os/IncidentReportArgs.h +++ b/libs/incident/include/android/os/IncidentReportArgs.h @@ -39,12 +39,13 @@ public: virtual status_t readFromParcel(const Parcel* in); void setAll(bool all); + void setDest(int dest); void addSection(int section); void addHeader(const vector& header); - inline bool all() const { return mAll; }; + inline bool all() const { return mAll; } bool containsSection(int section) const; - + inline int dest() const { return mDest; } inline const set& sections() const { return mSections; } inline const vector>& headers() const { return mHeaders; } @@ -54,6 +55,7 @@ private: set mSections; vector> mHeaders; bool mAll; + int mDest; }; } diff --git a/libs/incident/src/IncidentReportArgs.cpp b/libs/incident/src/IncidentReportArgs.cpp index f60490911aed0..e628722383871 100644 --- a/libs/incident/src/IncidentReportArgs.cpp +++ b/libs/incident/src/IncidentReportArgs.cpp @@ -25,14 +25,16 @@ namespace os { IncidentReportArgs::IncidentReportArgs() :mSections(), - mAll(false) + mAll(false), + mDest(-1) { } IncidentReportArgs::IncidentReportArgs(const IncidentReportArgs& that) :mSections(that.mSections), mHeaders(that.mHeaders), - mAll(that.mAll) + mAll(that.mAll), + mDest(that.mDest) { } @@ -74,6 +76,11 @@ IncidentReportArgs::writeToParcel(Parcel* out) const } } + err = out->writeInt32(mDest); + if (err != NO_ERROR) { + return err; + } + return NO_ERROR; } @@ -120,6 +127,13 @@ IncidentReportArgs::readFromParcel(const Parcel* in) } } + int32_t dest; + err = in->readInt32(&dest); + if (err != NO_ERROR) { + return err; + } + mDest = dest; + return OK; } @@ -132,6 +146,12 @@ IncidentReportArgs::setAll(bool all) } } +void +IncidentReportArgs::setDest(int dest) +{ + mDest = dest; +} + void IncidentReportArgs::addSection(int section) { From edfd5bb7888899976762623a4c940710026480ea Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 6 Sep 2017 17:09:11 -0700 Subject: [PATCH 2/2] Extract IncidentHeaderProto to a separated file for statsd to consume Create a HeaderSection to deal with header protos which is more testable Bug: 65422268 Test: unit tested Change-Id: Icdcbeded8bc963940a8f9e503cb65a9a247ca5b2 --- cmds/incidentd/src/Reporter.cpp | 36 +++---- cmds/incidentd/src/Reporter.h | 2 + cmds/incidentd/src/Section.cpp | 119 +++++++++++++-------- cmds/incidentd/src/Section.h | 12 +++ cmds/incidentd/tests/Section_test.cpp | 40 ++++++- core/proto/android/os/incident.proto | 12 +-- core/proto/android/os/incidentheader.proto | 34 ++++++ 7 files changed, 174 insertions(+), 81 deletions(-) create mode 100644 core/proto/android/os/incidentheader.proto diff --git a/cmds/incidentd/src/Reporter.cpp b/cmds/incidentd/src/Reporter.cpp index 722bd4fa8b873..11347e22d88e3 100644 --- a/cmds/incidentd/src/Reporter.cpp +++ b/cmds/incidentd/src/Reporter.cpp @@ -17,8 +17,6 @@ #define LOG_TAG "incidentd" #include "Reporter.h" -#include "io_util.h" -#include "protobuf.h" #include "report_directory.h" #include "section_list.h" @@ -52,6 +50,12 @@ ReportRequest::~ReportRequest() { } +bool +ReportRequest::ok() +{ + return fd >= 0 && err == NO_ERROR; +} + // ================================================================================ ReportRequestSet::ReportRequestSet() :mRequests(), @@ -117,6 +121,7 @@ Reporter::runReport() status_t err = NO_ERROR; bool needMainFd = false; int mainFd = -1; + HeaderSection headers; // See if we need the main file for (ReportRequestSet::iterator it=batch.begin(); it!=batch.end(); it++) { @@ -129,7 +134,7 @@ Reporter::runReport() // Create the directory if (!isTest) err = create_directory(mIncidentDirectory); if (err != NO_ERROR) { - goto done; + goto DONE; } // If there are too many files in the directory (for whatever reason), @@ -140,7 +145,7 @@ Reporter::runReport() // Open the file. err = create_file(&mainFd); if (err != NO_ERROR) { - goto done; + goto DONE; } // Add to the set @@ -155,24 +160,7 @@ Reporter::runReport() } // Write the incident headers - for (ReportRequestSet::iterator it=batch.begin(); it!=batch.end(); it++) { - const sp request = (*it); - const vector>& headers = request->args.headers(); - - for (vector>::const_iterator buf=headers.begin(); buf!=headers.end(); - buf++) { - int fd = request->fd >= 0 ? request->fd : mainFd; - - uint8_t buffer[20]; - uint8_t* p = write_length_delimited_tag_header(buffer, FIELD_ID_INCIDENT_HEADER, - buf->size()); - write_all(fd, buffer, p-buffer); - - write_all(fd, (uint8_t const*)buf->data(), buf->size()); - // If there was an error now, there will be an error later and we will remove - // it from the list then. - } - } + headers.Execute(&batch); // For each of the report fields, see if we need it, and if so, execute the command // and report to those that care that we're doing it. @@ -193,7 +181,7 @@ Reporter::runReport() if (err != NO_ERROR) { ALOGW("Incident section %s (%d) failed. Stopping report.", (*section)->name.string(), id); - goto done; + goto DONE; } // Notify listener of starting @@ -207,7 +195,7 @@ Reporter::runReport() } } -done: +DONE: // Close the file. if (mainFd >= 0) { close(mainFd); diff --git a/cmds/incidentd/src/Reporter.h b/cmds/incidentd/src/Reporter.h index b74cc42903062..2615c6202d3dd 100644 --- a/cmds/incidentd/src/Reporter.h +++ b/cmds/incidentd/src/Reporter.h @@ -40,6 +40,8 @@ struct ReportRequest : public virtual RefBase ReportRequest(const IncidentReportArgs& args, const sp &listener, int fd); virtual ~ReportRequest(); + + bool ok(); // returns true if the request is ok for write. }; // ================================================================================ diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp index 08959263838d2..6f052deaecf2c 100644 --- a/cmds/incidentd/src/Section.cpp +++ b/cmds/incidentd/src/Section.cpp @@ -39,7 +39,7 @@ const struct timespec WAIT_INTERVAL_NS = {0, 200 * 1000 * 1000}; const char* INCIDENT_HELPER = "/system/bin/incident_helper"; static pid_t -forkAndExecuteIncidentHelper(const int id, const char* name, Fpipe& p2cPipe, Fpipe& c2pPipe) +fork_execute_incident_helper(const int id, const char* name, Fpipe& p2cPipe, Fpipe& c2pPipe) { const char* ihArgs[] { INCIDENT_HELPER, "-s", String8::format("%d", id).string(), NULL }; @@ -74,14 +74,14 @@ forkAndExecuteIncidentHelper(const int id, const char* name, Fpipe& p2cPipe, Fpi } // ================================================================================ -static status_t killChild(pid_t pid) { +static status_t kill_child(pid_t pid) { int status; kill(pid, SIGKILL); if (waitpid(pid, &status, 0) == -1) return -1; return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status); } -static status_t waitForChild(pid_t pid) { +static status_t wait_child(pid_t pid) { int status; bool died = false; // wait for child to report status up to 1 seconds @@ -90,13 +90,12 @@ static status_t waitForChild(pid_t pid) { // sleep for 0.2 second nanosleep(&WAIT_INTERVAL_NS, NULL); } - if (!died) return killChild(pid); + if (!died) return kill_child(pid); return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status); } - // ================================================================================ static const Privacy* -GetPrivacyOfSection(int id) +get_privacy_of_section(int id) { if (id < 0) return NULL; int i=0; @@ -109,33 +108,27 @@ GetPrivacyOfSection(int id) return NULL; } +// ================================================================================ static status_t -WriteToRequest(const int id, const int fd, EncodedBuffer& buffer) +write_section_header(int fd, int sectionId, size_t size) { - if (buffer.size() == 0) return NO_ERROR; - - status_t err = NO_ERROR; uint8_t buf[20]; - uint8_t *p = write_length_delimited_tag_header(buf, id, buffer.size()); - err = write_all(fd, buf, p-buf); - if (err == NO_ERROR) { - err = buffer.flush(fd); - } - return err; + uint8_t *p = write_length_delimited_tag_header(buf, sectionId, size); + return write_all(fd, buf, p-buf); } static status_t -WriteToReportRequests(const int id, const FdBuffer& buffer, ReportRequestSet* requests) +write_report_requests(const int id, const FdBuffer& buffer, ReportRequestSet* requests) { status_t err = -EBADF; - EncodedBuffer encodedBuffer(buffer, GetPrivacyOfSection(id)); + EncodedBuffer encodedBuffer(buffer, get_privacy_of_section(id)); int writeable = 0; // The streaming ones, group requests by spec in order to save unnecessary strip operations map>> requestsBySpec; for (ReportRequestSet::iterator it = requests->begin(); it != requests->end(); it++) { sp request = *it; - if (!request->args.containsSection(id) || request->fd < 0 || request->err != NO_ERROR) { + if (!request->ok() || !request->args.containsSection(id)) { continue; // skip invalid request } PrivacySpec spec = new_spec_from_args(request->args.dest()); @@ -150,12 +143,11 @@ WriteToReportRequests(const int id, const FdBuffer& buffer, ReportRequestSet* re for (vector>::iterator it = mit->second.begin(); it != mit->second.end(); it++) { sp request = *it; - err = WriteToRequest(id, request->fd, encodedBuffer); - if (err != NO_ERROR) { - request->err = err; - } else { - writeable++; - } + err = write_section_header(request->fd, id, encodedBuffer.size()); + if (err != NO_ERROR) { request->err = err; continue; } + err = encodedBuffer.flush(request->fd); + if (err != NO_ERROR) { request->err = err; continue; } + writeable++; ALOGD("Section %d flushed %zu bytes to fd %d with spec %d", id, encodedBuffer.size(), request->fd, spec.dest); } encodedBuffer.clear(); @@ -165,21 +157,25 @@ WriteToReportRequests(const int id, const FdBuffer& buffer, ReportRequestSet* re if (requests->mainFd() >= 0) { err = encodedBuffer.strip(get_default_dropbox_spec()); if (err != NO_ERROR) return err; // the buffer data is corrupted. + if (encodedBuffer.size() == 0) goto DONE; - err = WriteToRequest(id, requests->mainFd(), encodedBuffer); - if (err != NO_ERROR) { - requests->setMainFd(-1); - } else { - writeable++; - } + err = write_section_header(requests->mainFd(), id, encodedBuffer.size()); + if (err != NO_ERROR) { requests->setMainFd(-1); goto DONE; } + err = encodedBuffer.flush(requests->mainFd()); + if (err != NO_ERROR) { requests->setMainFd(-1); goto DONE; } + writeable++; + ALOGD("Section %d flushed %zu bytes to dropbox %d", id, encodedBuffer.size(), requests->mainFd()); } + +DONE: // only returns error if there is no fd to write to. return writeable > 0 ? NO_ERROR : err; } // ================================================================================ Section::Section(int i, const int64_t timeoutMs) - :id(i), timeoutMs(timeoutMs) + :id(i), + timeoutMs(timeoutMs) { } @@ -187,9 +183,42 @@ Section::~Section() { } +// ================================================================================ +HeaderSection::HeaderSection() + :Section(FIELD_ID_INCIDENT_HEADER, 0) +{ +} + +HeaderSection::~HeaderSection() +{ +} + +status_t +HeaderSection::Execute(ReportRequestSet* requests) const +{ + for (ReportRequestSet::iterator it=requests->begin(); it!=requests->end(); it++) { + const sp request = *it; + const vector>& headers = request->args.headers(); + + for (vector>::const_iterator buf=headers.begin(); buf!=headers.end(); buf++) { + if (buf->empty()) continue; + + // So the idea is only requests with negative fd are written to dropbox file. + int fd = request->fd >= 0 ? request->fd : requests->mainFd(); + write_section_header(fd, FIELD_ID_INCIDENT_HEADER, buf->size()); + write_all(fd, (uint8_t const*)buf->data(), buf->size()); + // If there was an error now, there will be an error later and we will remove + // it from the list then. + } + } + return NO_ERROR; +} + // ================================================================================ FileSection::FileSection(int id, const char* filename, const int64_t timeoutMs) - : Section(id, timeoutMs), mFilename(filename) { + :Section(id, timeoutMs), + mFilename(filename) +{ name = filename; } @@ -215,7 +244,7 @@ FileSection::Execute(ReportRequestSet* requests) const return -errno; } - pid_t pid = forkAndExecuteIncidentHelper(this->id, this->name.string(), p2cPipe, c2pPipe); + pid_t pid = fork_execute_incident_helper(this->id, this->name.string(), p2cPipe, c2pPipe); if (pid == -1) { ALOGW("FileSection '%s' failed to fork", this->name.string()); return -errno; @@ -227,11 +256,11 @@ FileSection::Execute(ReportRequestSet* requests) const if (readStatus != NO_ERROR || buffer.timedOut()) { ALOGW("FileSection '%s' failed to read data from incident helper: %s, timedout: %s, kill: %s", this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false", - strerror(-killChild(pid))); + strerror(-kill_child(pid))); return readStatus; } - status_t ihStatus = waitForChild(pid); + status_t ihStatus = wait_child(pid); if (ihStatus != NO_ERROR) { ALOGW("FileSection '%s' abnormal child process: %s", this->name.string(), strerror(-ihStatus)); return ihStatus; @@ -239,7 +268,7 @@ FileSection::Execute(ReportRequestSet* requests) const ALOGD("FileSection '%s' wrote %zd bytes in %d ms", this->name.string(), buffer.size(), (int)buffer.durationMs()); - status_t err = WriteToReportRequests(this->id, buffer, requests); + status_t err = write_report_requests(this->id, buffer, requests); if (err != NO_ERROR) { ALOGW("FileSection '%s' failed writing: %s", this->name.string(), strerror(-err)); return err; @@ -396,7 +425,7 @@ WorkerThreadSection::Execute(ReportRequestSet* requests) const // Write the data that was collected ALOGD("WorkerThreadSection '%s' wrote %zd bytes in %d ms", name.string(), buffer.size(), (int)buffer.durationMs()); - err = WriteToReportRequests(this->id, buffer, requests); + err = write_report_requests(this->id, buffer, requests); if (err != NO_ERROR) { ALOGW("WorkerThreadSection '%s' failed writing: '%s'", this->name.string(), strerror(-err)); return err; @@ -433,7 +462,7 @@ CommandSection::init(const char* command, va_list args) } CommandSection::CommandSection(int id, const int64_t timeoutMs, const char* command, ...) - : Section(id, timeoutMs) + :Section(id, timeoutMs) { va_list args; va_start(args, command); @@ -442,7 +471,7 @@ CommandSection::CommandSection(int id, const int64_t timeoutMs, const char* comm } CommandSection::CommandSection(int id, const char* command, ...) - : Section(id) + :Section(id) { va_list args; va_start(args, command); @@ -484,7 +513,7 @@ CommandSection::Execute(ReportRequestSet* requests) const ALOGW("CommandSection '%s' failed in executing command: %s", this->name.string(), strerror(errno)); _exit(err); // exit with command error code } - pid_t ihPid = forkAndExecuteIncidentHelper(this->id, this->name.string(), cmdPipe, ihPipe); + pid_t ihPid = fork_execute_incident_helper(this->id, this->name.string(), cmdPipe, ihPipe); if (ihPid == -1) { ALOGW("CommandSection '%s' failed to fork", this->name.string()); return -errno; @@ -496,14 +525,14 @@ CommandSection::Execute(ReportRequestSet* requests) const ALOGW("CommandSection '%s' failed to read data from incident helper: %s, " "timedout: %s, kill command: %s, kill incident helper: %s", this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false", - strerror(-killChild(cmdPid)), strerror(-killChild(ihPid))); + strerror(-kill_child(cmdPid)), strerror(-kill_child(ihPid))); return readStatus; } // TODO: wait for command here has one trade-off: the failed status of command won't be detected until // buffer timeout, but it has advatage on starting the data stream earlier. - status_t cmdStatus = waitForChild(cmdPid); - status_t ihStatus = waitForChild(ihPid); + status_t cmdStatus = wait_child(cmdPid); + status_t ihStatus = wait_child(ihPid); if (cmdStatus != NO_ERROR || ihStatus != NO_ERROR) { ALOGW("CommandSection '%s' abnormal child processes, return status: command: %s, incident helper: %s", this->name.string(), strerror(-cmdStatus), strerror(-ihStatus)); @@ -512,7 +541,7 @@ CommandSection::Execute(ReportRequestSet* requests) const ALOGD("CommandSection '%s' wrote %zd bytes in %d ms", this->name.string(), buffer.size(), (int)buffer.durationMs()); - status_t err = WriteToReportRequests(this->id, buffer, requests); + status_t err = write_report_requests(this->id, buffer, requests); if (err != NO_ERROR) { ALOGW("CommandSection '%s' failed writing: %s", this->name.string(), strerror(-err)); return err; diff --git a/cmds/incidentd/src/Section.h b/cmds/incidentd/src/Section.h index 9cfd696c17f00..0a1e03eb6f410 100644 --- a/cmds/incidentd/src/Section.h +++ b/cmds/incidentd/src/Section.h @@ -44,6 +44,18 @@ public: virtual status_t Execute(ReportRequestSet* requests) const = 0; }; +/** + * Section that generates incident headers. + */ +class HeaderSection : public Section +{ +public: + HeaderSection(); + virtual ~HeaderSection(); + + virtual status_t Execute(ReportRequestSet* requests) const; +}; + /** * Section that reads in a file. */ diff --git a/cmds/incidentd/tests/Section_test.cpp b/cmds/incidentd/tests/Section_test.cpp index f2aee8fa2bb07..25b05b2b15180 100644 --- a/cmds/incidentd/tests/Section_test.cpp +++ b/cmds/incidentd/tests/Section_test.cpp @@ -39,6 +39,8 @@ using ::testing::StrEq; using ::testing::internal::CaptureStdout; using ::testing::internal::GetCapturedStdout; +// NOTICE: this test requires /system/bin/incident_helper is installed. + class SimpleListener : public IIncidentReportStatusListener { public: @@ -54,7 +56,43 @@ protected: virtual IBinder* onAsBinder() override { return nullptr; }; }; -// NOTICE: this test requires /system/bin/incident_helper is installed. +TEST(SectionTest, HeaderSection) { + TemporaryFile output2; + HeaderSection hs; + ReportRequestSet requests; + + IncidentReportArgs args1, args2; + args1.addSection(1); + args1.addSection(2); + args2.setAll(true); + + vector head1; + head1.push_back('a'); + head1.push_back('x'); + head1.push_back('e'); + + vector head2; + head2.push_back('p'); + head2.push_back('u'); + head2.push_back('p'); + + args1.addHeader(head1); + args1.addHeader(head2); + args2.addHeader(head2); + + requests.add(new ReportRequest(args1, new SimpleListener(), -1)); + requests.add(new ReportRequest(args2, new SimpleListener(), output2.fd)); + requests.setMainFd(STDOUT_FILENO); + + string content; + CaptureStdout(); + ASSERT_EQ(NO_ERROR, hs.Execute(&requests)); + EXPECT_THAT(GetCapturedStdout(), StrEq("\n\x3" "axe\n\x03pup")); + + EXPECT_TRUE(ReadFileToString(output2.path, &content)); + EXPECT_THAT(content, StrEq("\n\x03pup")); +} + TEST(SectionTest, FileSection) { TemporaryFile tf; FileSection fs(REVERSE_PARSER, tf.path); diff --git a/core/proto/android/os/incident.proto b/core/proto/android/os/incident.proto index 5dfcd2a63b18f..aab4142d252fc 100644 --- a/core/proto/android/os/incident.proto +++ b/core/proto/android/os/incident.proto @@ -31,22 +31,12 @@ import "frameworks/base/core/proto/android/service/package.proto"; import "frameworks/base/core/proto/android/service/power.proto"; import "frameworks/base/core/proto/android/service/print.proto"; import "frameworks/base/core/proto/android/providers/settings.proto"; +import "frameworks/base/core/proto/android/os/incidentheader.proto"; import "frameworks/base/core/proto/android/os/kernelwake.proto"; import "frameworks/base/core/proto/android/os/procrank.proto"; package android.os; -message IncidentHeaderProto { - enum Cause { - CAUSE_UNKNOWN = 0; - CAUSE_USER = 1; - CAUSE_ANR = 2; - CAUSE_CRASH = 3; - } - - Cause cause = 1; -} - // privacy field options must not be set at this level because all // the sections are able to be controlled and configured by section ids. // Instead privacy field options need to be configured in each section proto message. diff --git a/core/proto/android/os/incidentheader.proto b/core/proto/android/os/incidentheader.proto new file mode 100644 index 0000000000000..55a06162dd2fe --- /dev/null +++ b/core/proto/android/os/incidentheader.proto @@ -0,0 +1,34 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +syntax = "proto3"; + +option java_multiple_files = true; +option java_outer_classname = "IncidentHeaderProtoMetadata"; + +package android.os; + +message IncidentHeaderProto { + enum Cause { + CAUSE_UNKNOWN = 0; + CAUSE_USER = 1; + CAUSE_ANR = 2; + CAUSE_CRASH = 3; + } + + Cause cause = 1; +} +