Add more information to incident header. Especially add config keys
to check if the report is uploadable.
Move incidentheader.proto to libincident so statds is able to include a lite
proto class for incident header.
Change IncidentReportArgs to add the proto object instead of serialized
bytes to prevent caller gives meaningless data.
Bug: 70241842
Test: push config to statsd and verify incidentd generate the report
with correct header.
Change-Id: If95b655be71047b019b229e5903a08f3c21a1f29
diff --git a/Android.bp b/Android.bp
index 19c0580..641ce5f 100644
--- a/Android.bp
+++ b/Android.bp
@@ -689,7 +689,10 @@
" $(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 298b85d..8ae0350a 100644
--- a/Android.mk
+++ b/Android.mk
@@ -797,7 +797,8 @@
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)
@@ -809,7 +810,8 @@
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 8420bc8..368a70b 100644
--- a/cmds/incidentd/Android.mk
+++ b/cmds/incidentd/Android.mk
@@ -134,6 +134,7 @@
libcutils \
libincident \
liblog \
+ libprotobuf-cpp-lite \
libprotoutil \
libselinux \
libservices \
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index a97eb86..1d5ab59 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -45,22 +45,27 @@
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 531c9f2..c494bd6 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 @@
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 @@
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, 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 cbfb896..2cfd7df 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 @@
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 @@
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 @@
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 f10b2cf..39a19a3 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
@@ -19,6 +19,7 @@
#include "AnomalyTracker.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>
@@ -253,10 +254,10 @@
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 2752a7e..e5efb90 100644
--- a/core/proto/android/os/incident.proto
+++ b/core/proto/android/os/incident.proto
@@ -21,7 +21,6 @@
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/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/core/proto/android/os/incidentheader.proto b/core/proto/android/os/incidentheader.proto
deleted file mode 100644
index f0c736a..0000000
--- a/core/proto/android/os/incidentheader.proto
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- * 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 = "proto2";
-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.
-message IncidentHeaderProto {
-
- // From statsd config, the name of the anomaly, usually human readable.
- optional string incident_name = 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.
- 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.
- message StatsdConfigKey {
- optional int32 uid = 1;
- optional string name = 2;
- }
- optional StatsdConfigKey config_key = 6;
-}
diff --git a/libs/incident/Android.mk b/libs/incident/Android.mk
index 5f3e407..b63400f 100644
--- a/libs/incident/Android.mk
+++ b/libs/incident/Android.mk
@@ -32,6 +32,7 @@
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 2849d58..c56f689 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 @@
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/libs/incident/proto/android/os/header.proto b/libs/incident/proto/android/os/header.proto
new file mode 100644
index 0000000..a84dc48
--- /dev/null
+++ b/libs/incident/proto/android/os/header.proto
@@ -0,0 +1,40 @@
+/*
+ * 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 = "proto2";
+option java_multiple_files = true;
+
+package android.os;
+
+// 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 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 input clicking the
+ // bug-report button.
+ optional string reason = 2;
+
+ // 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; // The uid pushes the config to statsd.
+ optional int64 id = 2; // The unique id of the statsd config.
+ }
+ optional StatsdConfigKey config_key = 3;
+}
diff --git a/libs/incident/src/IncidentReportArgs.cpp b/libs/incident/src/IncidentReportArgs.cpp
index bd9c8ee..fbc21e5 100644
--- a/libs/incident/src/IncidentReportArgs.cpp
+++ b/libs/incident/src/IncidentReportArgs.cpp
@@ -161,8 +161,14 @@
}
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);
}