Get incidentd cts working again.
- The buffer size increased, and the CTS test that checked that
was triggering.
- Privacy filtering wasn't working for the stack trace sections
- The incident command was not handling the default arguments correctly
- The throttler was throttling streaming reports, which made the
test flaky.
Bug: 126253679
Test: atest CtsIncidentHostTestCases
Change-Id: I342cd7d0421ea8c22b7796fc99e779f21855af73
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index b4021d1b..71b89a2 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -174,12 +174,11 @@
}
void ReportHandler::take_report() {
- // Cycle the batch
+ // Cycle the batch and throttle.
sp<ReportBatch> batch;
{
unique_lock<mutex> lock(mLock);
- batch = mBatch;
- mBatch = new ReportBatch();
+ batch = mThrottler->filterBatch(mBatch);
}
if (batch->empty()) {
@@ -189,13 +188,6 @@
sp<Reporter> reporter = new Reporter(mWorkDirectory, batch);
- // TODO: Do we really want to clear the reports if we throttle? Should we only throttle
- // requests going to dropbox? How do we reconcile throttling with testing?
- if (mThrottler->shouldThrottle()) {
- ALOGW("RunReport got throttled.");
- return;
- }
-
// Take the report, which might take a while. More requests might queue
// up while we're doing this, and we'll handle them in their next batch.
// TODO: We should further rate-limit the reports to no more than N per time-period.
@@ -203,7 +195,13 @@
size_t reportByteSize = 0;
reporter->runReport(&reportByteSize);
- mThrottler->addReportSize(reportByteSize);
+ // Tell the throttler how big it was, for the next throttling.
+ // TODO: This still isn't ideal. The throttler really should just track the
+ // persisted reqeusts, but changing Reporter::runReport() to track that individually
+ // will be a big change.
+ if (batch->hasPersistedReports()) {
+ mThrottler->addReportSize(reportByteSize);
+ }
// Kick off the next steps, one of which is to send any new or otherwise remaining
// approvals, and one of which is to send any new or remaining broadcasts.
@@ -247,11 +245,11 @@
IncidentService::~IncidentService() {}
Status IncidentService::reportIncident(const IncidentReportArgs& args) {
- // TODO: Validate that the privacy policy is one of the real ones.
- // If it isn't, clamp it to the next more restrictive real one.
+ IncidentReportArgs argsCopy(args);
- // TODO: This function should reject the LOCAL privacy policy.
- // Those have to stream.
+ // Validate that the privacy policy is one of the real ones.
+ // If it isn't, clamp it to the next more restrictive real one.
+ argsCopy.setPrivacyPolicy(cleanup_privacy_policy(args.getPrivacyPolicy()));
// TODO: Check that the broadcast recevier has the proper permissions
// TODO: Maybe we should consider relaxing the permissions if it's going to
@@ -261,8 +259,15 @@
return status;
}
+ // If they asked for the LOCAL privacy policy, give them EXPLICT. LOCAL has to
+ // be streamed. (This only applies to shell/root, because everyone else would have
+ // been rejected by checkIncidentPermissions()).
+ if (argsCopy.getPrivacyPolicy() < PRIVACY_POLICY_EXPLICIT) {
+ ALOGI("Demoting privacy policy to EXPLICT for persisted report.");
+ argsCopy.setPrivacyPolicy(PRIVACY_POLICY_EXPLICIT);
+ }
+
// If they didn't specify a component, use dropbox.
- IncidentReportArgs argsCopy(args);
if (argsCopy.receiverPkg().length() == 0 && argsCopy.receiverCls().length() == 0) {
argsCopy.setReceiverPkg(DROPBOX_SENTINEL.getPackageName());
argsCopy.setReceiverCls(DROPBOX_SENTINEL.getClassName());
@@ -276,22 +281,21 @@
Status IncidentService::reportIncidentToStream(const IncidentReportArgs& args,
const sp<IIncidentReportStatusListener>& listener,
const unique_fd& stream) {
- // TODO: Validate that the privacy policy is one of the real ones.
- // If it isn't, clamp it to the next more restrictive real one.
-
- // TODO: Only shell should be able to do a LOCAL privacy policy report.
+ IncidentReportArgs argsCopy(args);
// Streaming reports can not also be broadcast.
- IncidentReportArgs argsCopy(args);
argsCopy.setReceiverPkg("");
argsCopy.setReceiverCls("");
+ // Validate that the privacy policy is one of the real ones.
+ // If it isn't, clamp it to the next more restrictive real one.
+ argsCopy.setPrivacyPolicy(cleanup_privacy_policy(args.getPrivacyPolicy()));
+
Status status = checkIncidentPermissions(argsCopy);
if (!status.isOk()) {
return status;
}
-
// The ReportRequest takes ownership of the fd, so we need to dup it.
int fd = dup(stream.get());
if (fd < 0) {
diff --git a/cmds/incidentd/src/Privacy.cpp b/cmds/incidentd/src/Privacy.cpp
index 4fe74c4..0cc358f 100644
--- a/cmds/incidentd/src/Privacy.cpp
+++ b/cmds/incidentd/src/Privacy.cpp
@@ -18,17 +18,30 @@
#include <android/os/IncidentReportArgs.h>
#include <stdlib.h>
+#include <strstream>
+
namespace android {
namespace os {
namespace incidentd {
using namespace android::os;
+using std::strstream;
static const bool kEncryptionEnabled = false;
uint64_t encode_field_id(const Privacy* p) { return (uint64_t)p->type << 32 | p->field_id; }
+string Privacy::toString() const {
+ if (this == NULL) {
+ return "Privacy{null}";
+ }
+ strstream os;
+ os << "Privacy{field_id=" << field_id << " type=" << ((int)type)
+ << " children=" << ((void*)children) << " policy=" << ((int)policy) << "}";
+ return os.str();
+}
+
const Privacy* lookup(const Privacy* p, uint32_t fieldId) {
if (p->children == NULL) return NULL;
for (int i = 0; p->children[i] != NULL; i++) { // NULL-terminated.
@@ -87,6 +100,16 @@
return mPolicy == android::os::PRIVACY_POLICY_LOCAL;
}
+uint8_t cleanup_privacy_policy(uint8_t policy) {
+ if (policy >= PRIVACY_POLICY_AUTOMATIC) {
+ return PRIVACY_POLICY_AUTOMATIC;
+ }
+ if (policy >= PRIVACY_POLICY_EXPLICIT) {
+ return PRIVACY_POLICY_EXPLICIT;
+ }
+ return PRIVACY_POLICY_LOCAL;
+}
+
} // namespace incidentd
} // namespace os
} // namespace android
diff --git a/cmds/incidentd/src/Privacy.h b/cmds/incidentd/src/Privacy.h
index b599c1c4..9cde748 100644
--- a/cmds/incidentd/src/Privacy.h
+++ b/cmds/incidentd/src/Privacy.h
@@ -50,8 +50,11 @@
// DESTINATION Enum in frameworks/base/core/proto/android/privacy.proto.
uint8_t policy;
+
// A list of regexp rules for stripping string fields in proto.
const char** patterns;
+
+ string toString() const;
};
// Encode field id used by ProtoOutputStream.
@@ -90,6 +93,11 @@
// TODO: Add privacy flag in incident.proto and auto generate it inside Privacy.
bool sectionEncryption(int section_id);
+/**
+ * If a privacy policy is other than the defined values, update it to a real one.
+ */
+uint8_t cleanup_privacy_policy(uint8_t policy);
+
} // namespace incidentd
} // namespace os
} // namespace android
diff --git a/cmds/incidentd/src/PrivacyFilter.cpp b/cmds/incidentd/src/PrivacyFilter.cpp
index 14ddec1..ca6fb37 100644
--- a/cmds/incidentd/src/PrivacyFilter.cpp
+++ b/cmds/incidentd/src/PrivacyFilter.cpp
@@ -192,7 +192,7 @@
ProtoOutputStream proto;
// Optimization when no strip happens.
- if (mRestrictions == NULL || mRestrictions->children == NULL || spec.RequireAll()) {
+ if (mRestrictions == NULL || spec.RequireAll()) {
if (spec.CheckPremission(mRestrictions)) {
mSize = mData->size();
}
diff --git a/cmds/incidentd/src/Reporter.cpp b/cmds/incidentd/src/Reporter.cpp
index 00a31e0..dc4065b 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -286,6 +286,22 @@
mPersistedRequests.clear();
}
+void ReportBatch::transferStreamingRequests(const sp<ReportBatch>& that) {
+ for (vector<sp<ReportRequest>>::iterator request = mStreamingRequests.begin();
+ request != mStreamingRequests.end(); request++) {
+ that->mStreamingRequests.push_back(*request);
+ }
+ mStreamingRequests.clear();
+}
+
+void ReportBatch::transferPersistedRequests(const sp<ReportBatch>& that) {
+ for (map<ComponentName, sp<ReportRequest>>::iterator it = mPersistedRequests.begin();
+ it != mPersistedRequests.end(); it++) {
+ that->mPersistedRequests[it->first] = it->second;
+ }
+ mPersistedRequests.clear();
+}
+
void ReportBatch::getFailedRequests(vector<sp<ReportRequest>>* requests) {
for (map<ComponentName, sp<ReportRequest>>::iterator it = mPersistedRequests.begin();
it != mPersistedRequests.end(); it++) {
diff --git a/cmds/incidentd/src/Reporter.h b/cmds/incidentd/src/Reporter.h
index 5179f48..fb3961a 100644
--- a/cmds/incidentd/src/Reporter.h
+++ b/cmds/incidentd/src/Reporter.h
@@ -16,7 +16,6 @@
#pragma once
#include "FdBuffer.h"
-#include "Throttler.h"
#include "WorkDirectory.h"
#include "frameworks/base/core/proto/android/os/metadata.pb.h"
@@ -155,6 +154,18 @@
void clearPersistedRequests();
/**
+ * Move the streaming requests in this batch to that batch. After this call there
+ * will be no streaming requests in this batch.
+ */
+ void transferStreamingRequests(const sp<ReportBatch>& that);
+
+ /**
+ * Move the persisted requests in this batch to that batch. After this call there
+ * will be no streaming requests in this batch.
+ */
+ void transferPersistedRequests(const sp<ReportBatch>& that);
+
+ /**
* Get the requests that have encountered errors.
*/
void getFailedRequests(vector<sp<ReportRequest>>* requests);
diff --git a/cmds/incidentd/src/Throttler.cpp b/cmds/incidentd/src/Throttler.cpp
index 11136ec..5f90ab3 100644
--- a/cmds/incidentd/src/Throttler.cpp
+++ b/cmds/incidentd/src/Throttler.cpp
@@ -33,6 +33,21 @@
Throttler::~Throttler() {}
+sp<ReportBatch> Throttler::filterBatch(const sp<ReportBatch>& queued) {
+ sp<ReportBatch> result = new ReportBatch();
+
+ // We will never throttle the streaming ones.
+ queued->transferStreamingRequests(result);
+
+ // If the persisted ones aren't to be throttled, then add them to the
+ // batch we're going to do.
+ if (!shouldThrottle()) {
+ queued->transferPersistedRequests(result);
+ }
+
+ return result;
+}
+
bool Throttler::shouldThrottle() {
int64_t now = android::elapsedRealtime();
if (now > mRefractoryPeriodMs + mLastRefractoryMs) {
@@ -56,4 +71,4 @@
} // namespace incidentd
} // namespace os
-} // namespace android
\ No newline at end of file
+} // namespace android
diff --git a/cmds/incidentd/src/Throttler.h b/cmds/incidentd/src/Throttler.h
index e8f317d7..46a1472 100644
--- a/cmds/incidentd/src/Throttler.h
+++ b/cmds/incidentd/src/Throttler.h
@@ -14,16 +14,19 @@
* limitations under the License.
*/
-#ifndef THROTTLER_H
-#define THROTTLER_H
+#pragma once
+
+#include "Reporter.h"
#include <utils/RefBase.h>
+#include <vector>
#include <unistd.h>
namespace android {
namespace os {
namespace incidentd {
+
/**
* This is a size-based throttler which prevents incidentd to take more data.
*/
@@ -33,8 +36,11 @@
~Throttler();
/**
- * Asserts this before starting taking report.
+ * Return a batch containing reports, if any that should be executed.
+ * Those will be removed from 'queued'.
*/
+ sp<ReportBatch> filterBatch(const sp<ReportBatch>& queued);
+
bool shouldThrottle();
void addReportSize(size_t reportByteSize);
@@ -52,5 +58,3 @@
} // namespace incidentd
} // namespace os
} // namespace android
-
-#endif // THROTTLER_H