Merge "Add more information to incident header. Especially add config keys to check if the report is uploadable."

12 files changed, 63 insertions, 49 deletions

diff --git a/Android.bp b/Android.bp
index 19c0580e21d2..641ce5fdff3c 100644
--- a/Android.bp
+++ b/Android.bp
@@ -689,7 +689,10 @@ gensrcs {
         "  $(in) " +
         "&& $(location soong_zip) -jar -o $(out) -C $(genDir)/$(in) -D $(genDir)/$(in)",
 
-    srcs: ["core/proto/**/*.proto"],
+    srcs: [
+        "core/proto/**/*.proto",
+        "libs/incident/**/*.proto",
+    ],
     output_extension: "srcjar",
 }
 
diff --git a/Android.mk b/Android.mk
index d823be2ef9f3..2254008422e0 100644
--- a/Android.mk
+++ b/Android.mk
@@ -806,7 +806,8 @@ LOCAL_PROTO_JAVA_OUTPUT_PARAMS := \
     store_unknown_fields = true
 LOCAL_JAVA_LIBRARIES := core-oj core-libart
 LOCAL_SRC_FILES := \
-    $(call all-proto-files-under, core/proto)
+    $(call all-proto-files-under, core/proto) \
+    $(call all-proto-files-under, libs/incident/proto/android/os)
 include $(BUILD_STATIC_JAVA_LIBRARY)
 
 
@@ -818,7 +819,8 @@ LOCAL_PROTOC_OPTIMIZE_TYPE := lite
 LOCAL_PROTOC_FLAGS := \
     -Iexternal/protobuf/src
 LOCAL_SRC_FILES := \
-    $(call all-proto-files-under, core/proto)
+    $(call all-proto-files-under, core/proto) \
+    $(call all-proto-files-under, libs/incident/proto/android/os)
 include $(BUILD_STATIC_JAVA_LIBRARY)
 
 # Include subdirectory makefiles
diff --git a/cmds/incidentd/Android.mk b/cmds/incidentd/Android.mk
index 8420bc8f7ac7..368a70bc3898 100644
--- a/cmds/incidentd/Android.mk
+++ b/cmds/incidentd/Android.mk
@@ -134,6 +134,7 @@ LOCAL_SHARED_LIBRARIES := \
     libcutils \
     libincident \
     liblog \
+    libprotobuf-cpp-lite \
     libprotoutil \
     libselinux \
     libservices \
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index a97eb861578e..1d5ab59f9ba8 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -45,22 +45,27 @@ String16 const USAGE_STATS_PERMISSION("android.permission.PACKAGE_USAGE_STATS");
 static Status
 checkIncidentPermissions(const IncidentReportArgs& args)
 {
+    uid_t callingUid = IPCThreadState::self()->getCallingUid();
+    if (callingUid == AID_ROOT || callingUid == AID_SHELL) {
+        // root doesn't have permission.DUMP if don't do this!
+        return Status::ok();
+    }
+
     // checking calling permission.
     if (!checkCallingPermission(DUMP_PERMISSION)) {
         ALOGW("Calling pid %d and uid %d does not have permission: android.permission.DUMP",
-                IPCThreadState::self()->getCallingPid(), IPCThreadState::self()->getCallingUid());
+                IPCThreadState::self()->getCallingPid(), callingUid);
         return Status::fromExceptionCode(Status::EX_SECURITY,
                 "Calling process does not have permission: android.permission.DUMP");
     }
     if (!checkCallingPermission(USAGE_STATS_PERMISSION)) {
         ALOGW("Calling pid %d and uid %d does not have permission: android.permission.USAGE_STATS",
-                IPCThreadState::self()->getCallingPid(), IPCThreadState::self()->getCallingUid());
+                IPCThreadState::self()->getCallingPid(), callingUid);
         return Status::fromExceptionCode(Status::EX_SECURITY,
                 "Calling process does not have permission: android.permission.USAGE_STATS");
     }
 
     // checking calling request uid permission.
-    uid_t callingUid = IPCThreadState::self()->getCallingUid();
     switch (args.dest()) {
         case DEST_LOCAL:
             if (callingUid != AID_SHELL || callingUid != AID_ROOT) {
diff --git a/cmds/incidentd/tests/Reporter_test.cpp b/cmds/incidentd/tests/Reporter_test.cpp
index 531c9f29bf03..c494bd646b0b 100644
--- a/cmds/incidentd/tests/Reporter_test.cpp
+++ b/cmds/incidentd/tests/Reporter_test.cpp
@@ -17,6 +17,7 @@
 #include "Reporter.h"
 
 #include <android/os/BnIncidentReportStatusListener.h>
+#include <frameworks/base/libs/incident/proto/android/os/header.pb.h>
 
 #include <android-base/file.h>
 #include <android-base/test_utils.h>
@@ -29,6 +30,7 @@
 using namespace android;
 using namespace android::base;
 using namespace android::binder;
+using namespace android::os;
 using namespace std;
 using ::testing::StrEq;
 using ::testing::Test;
@@ -141,7 +143,8 @@ TEST_F(ReporterTest, RunReportWithHeaders) {
     IncidentReportArgs args1, args2;
     args1.addSection(1);
     args2.addSection(2);
-    std::vector<uint8_t> header {'a', 'b', 'c', 'd', 'e'};
+    IncidentHeaderProto header;
+    header.set_alert_id(12);
     args2.addHeader(header);
     sp<ReportRequest> r1 = new ReportRequest(args1, l, tf.fd);
     sp<ReportRequest> r2 = new ReportRequest(args2, l, tf.fd);
@@ -153,7 +156,7 @@ TEST_F(ReporterTest, RunReportWithHeaders) {
 
     string result;
     ReadFileToString(tf.path, &result);
-    EXPECT_THAT(result, StrEq("\n\x5" "abcde"));
+    EXPECT_THAT(result, StrEq("\n\x2" "\b\f"));
 
     EXPECT_EQ(l->startInvoked, 2);
     EXPECT_EQ(l->finishInvoked, 2);
@@ -164,13 +167,16 @@ TEST_F(ReporterTest, RunReportWithHeaders) {
 
 TEST_F(ReporterTest, RunReportToGivenDirectory) {
     IncidentReportArgs args;
-    args.addHeader({'1', '2', '3'});
-    args.addHeader({'a', 'b', 'c', 'd'});
+    IncidentHeaderProto header1, header2;
+    header1.set_alert_id(12);
+    header2.set_reason("abcd");
+    args.addHeader(header1);
+    args.addHeader(header2);
     sp<ReportRequest> r = new ReportRequest(args, l, -1);
     reporter->batch.add(r);
 
     ASSERT_EQ(Reporter::REPORT_FINISHED, reporter->runReport());
     vector<string> results = InspectFiles();
     ASSERT_EQ((int)results.size(), 1);
-    EXPECT_EQ(results[0], "\n\x3" "123\n\x4" "abcd");
+    EXPECT_EQ(results[0], "\n\x2" "\b\f\n\x6" "\x12\x4" "abcd");
 }
diff --git a/cmds/incidentd/tests/Section_test.cpp b/cmds/incidentd/tests/Section_test.cpp
index cbfb89685de0..2cfd7df6be84 100644
--- a/cmds/incidentd/tests/Section_test.cpp
+++ b/cmds/incidentd/tests/Section_test.cpp
@@ -18,6 +18,7 @@
 
 #include <android-base/file.h>
 #include <android-base/test_utils.h>
+#include <frameworks/base/libs/incident/proto/android/os/header.pb.h>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <string.h>
@@ -34,6 +35,7 @@ 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 android::os;
 using namespace std;
 using ::testing::StrEq;
 using ::testing::internal::CaptureStdout;
@@ -66,15 +68,9 @@ TEST(SectionTest, HeaderSection) {
     args1.addSection(2);
     args2.setAll(true);
 
-    vector<uint8_t> head1;
-    head1.push_back('a');
-    head1.push_back('x');
-    head1.push_back('e');
-
-    vector<uint8_t> head2;
-    head2.push_back('p');
-    head2.push_back('u');
-    head2.push_back('p');
+    IncidentHeaderProto head1, head2;
+    head1.set_reason("axe");
+    head2.set_reason("pup");
 
     args1.addHeader(head1);
     args1.addHeader(head2);
@@ -87,10 +83,10 @@ TEST(SectionTest, HeaderSection) {
     string content;
     CaptureStdout();
     ASSERT_EQ(NO_ERROR, hs.Execute(&requests));
-    EXPECT_THAT(GetCapturedStdout(), StrEq("\n\x3" "axe\n\x03pup"));
+    EXPECT_THAT(GetCapturedStdout(), StrEq("\n\x5" "\x12\x3" "axe\n\x05\x12\x03pup"));
 
     EXPECT_TRUE(ReadFileToString(output2.path, &content));
-    EXPECT_THAT(content, StrEq("\n\x03pup"));
+    EXPECT_THAT(content, StrEq("\n\x05\x12\x03pup"));
 }
 
 TEST(SectionTest, FileSection) {
diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.cpp b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
index 9b65acf86604..e34aed33a3cb 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
@@ -20,6 +20,7 @@
 #include "AnomalyTracker.h"
 #include "external/Perfetto.h"
 #include "guardrail/StatsdStats.h"
+#include "frameworks/base/libs/incident/proto/android/os/header.pb.h"
 
 #include <android/os/IIncidentManager.h>
 #include <android/os/IncidentReportArgs.h>
@@ -254,10 +255,10 @@ void AnomalyTracker::informSubscribers(const HashableDimensionKey& key) {
             for (const auto section : incidentdSections) {
                 incidentReport.addSection(section);
             }
-            int64_t alertId = mAlert.id();
-            std::vector<uint8_t> header;
-            uint8_t* src = static_cast<uint8_t*>(static_cast<void*>(&alertId));
-            header.insert(header.end(), src, src + sizeof(int64_t));
+            android::os::IncidentHeaderProto header;
+            header.set_alert_id(mAlert.id());
+            header.mutable_config_key()->set_uid(mConfigKey.GetUid());
+            header.mutable_config_key()->set_id(mConfigKey.GetId());
             incidentReport.addHeader(header);
             service->reportIncident(incidentReport);
         } else {
diff --git a/core/proto/android/os/incident.proto b/core/proto/android/os/incident.proto
index 2752a7e07ff4..e5efb90c922e 100644
--- a/core/proto/android/os/incident.proto
+++ b/core/proto/android/os/incident.proto
@@ -21,7 +21,6 @@ option java_outer_classname = "IncidentProtoMetadata";
 import "frameworks/base/core/proto/android/os/batterytype.proto";
 import "frameworks/base/core/proto/android/os/cpufreq.proto";
 import "frameworks/base/core/proto/android/os/cpuinfo.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/pagetypeinfo.proto";
 import "frameworks/base/core/proto/android/os/procrank.proto";
@@ -46,6 +45,7 @@ import "frameworks/base/core/proto/android/service/print.proto";
 import "frameworks/base/core/proto/android/service/procstats.proto";
 import "frameworks/base/core/proto/android/util/event_log_tags.proto";
 import "frameworks/base/core/proto/android/util/log.proto";
+import "frameworks/base/libs/incident/proto/android/os/header.proto";
 import "frameworks/base/libs/incident/proto/android/privacy.proto";
 import "frameworks/base/libs/incident/proto/android/section.proto";
 
diff --git a/libs/incident/Android.mk b/libs/incident/Android.mk
index 5f3e4071afef..b63400f14609 100644
--- a/libs/incident/Android.mk
+++ b/libs/incident/Android.mk
@@ -32,6 +32,7 @@ LOCAL_C_INCLUDES := \
 LOCAL_SRC_FILES := \
         ../../core/java/android/os/IIncidentManager.aidl \
         ../../core/java/android/os/IIncidentReportStatusListener.aidl \
+        proto/android/os/header.proto \
         src/IncidentReportArgs.cpp
 
 LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include
diff --git a/libs/incident/include/android/os/IncidentReportArgs.h b/libs/incident/include/android/os/IncidentReportArgs.h
index 2849d580d88b..c56f689b7419 100644
--- a/libs/incident/include/android/os/IncidentReportArgs.h
+++ b/libs/incident/include/android/os/IncidentReportArgs.h
@@ -24,6 +24,8 @@
 #include <set>
 #include <vector>
 
+#include "frameworks/base/libs/incident/proto/android/os/header.pb.h"
+
 namespace android {
 namespace os {
 
@@ -47,7 +49,7 @@ public:
     void setAll(bool all);
     void setDest(int dest);
     void addSection(int section);
-    void addHeader(const vector<uint8_t>& header);
+    void addHeader(const IncidentHeaderProto& headerProto);
 
     inline bool all() const { return mAll; }
     bool containsSection(int section) const;
diff --git a/core/proto/android/os/incidentheader.proto b/libs/incident/proto/android/os/header.proto
index f0c736a866a6..a84dc48b8b34 100644
--- a/core/proto/android/os/incidentheader.proto
+++ b/libs/incident/proto/android/os/header.proto
@@ -19,31 +19,22 @@ option java_multiple_files = true;
 
 package android.os;
 
-// IncidentHeaderProto contains extra information the caller of incidentd want to
-// attach in an incident report, the data should just be informative.
+// IncidentHeaderProto contains extra information the caller of incidentd wants
+// to attach in an incident report, the data should just be informative.
 message IncidentHeaderProto {
-
-    // From statsd config, the name of the anomaly, usually human readable.
-    optional string incident_name = 1;
+    // From statsd config, the id of the anomaly alert, unique among alerts.
+    optional int64 alert_id = 1;
 
     // Format a human readable reason why an incident report is requested.
-    // It's optional and may directly come from a user clicking the bug-report button.
+    // It's optional and may directly come from a user input clicking the
+    // bug-report button.
     optional string reason = 2;
 
-    // From statsd, the metric which causes the anomaly triggered.
-    optional string metric_name = 3;
-
-    // From statsd, the metric value exceeds the threshold. This is useful for
-    // ValueMetric and GaugeMetric.
-    oneof metric_value {
-        int64 int64_value = 4;
-        double double_value = 5;
-    }
-
-    // Defines which stats config used to fire the request.
+    // Defines which stats config used to fire the request, incident report will
+    // only be uploaded if this value is given.
     message StatsdConfigKey {
-        optional int32 uid = 1;
-        optional string name = 2;
+      optional int32 uid = 1; // The uid pushes the config to statsd.
+      optional int64 id = 2; // The unique id of the statsd config.
     }
-    optional StatsdConfigKey config_key = 6;
+    optional StatsdConfigKey config_key = 3;
 }
diff --git a/libs/incident/src/IncidentReportArgs.cpp b/libs/incident/src/IncidentReportArgs.cpp
index bd9c8eeb1d74..fbc21e558806 100644
--- a/libs/incident/src/IncidentReportArgs.cpp
+++ b/libs/incident/src/IncidentReportArgs.cpp
@@ -161,8 +161,14 @@ IncidentReportArgs::addSection(int section)
 }
 
 void
-IncidentReportArgs::addHeader(const vector<uint8_t>& header)
+IncidentReportArgs::addHeader(const IncidentHeaderProto& headerProto)
 {
+    vector<uint8_t> header;
+    auto serialized = headerProto.SerializeAsString();
+    if (serialized.empty()) return;
+    for (auto it = serialized.begin(); it != serialized.end(); it++) {
+        header.push_back((uint8_t)*it);
+    }
     mHeaders.push_back(header);
 }