Trace Redaction - Ftrace redaction framework
Expands the functionality of ftrace redaction handler to support a
wider range of redactions. This will thread merging to be implemented
as ftrace redactions.
Through this change, some primitives were updated to ensure uid
checks were using normalized uids.
Bug: 336807771
Bug: 337874700
Change-Id: I1e46aa5d049d0564fb4de8c7295b5c6f0dc51baf
diff --git a/src/trace_redaction/redact_ftrace_event.cc b/src/trace_redaction/redact_ftrace_event.cc
index 6c09236..a16a90d 100644
--- a/src/trace_redaction/redact_ftrace_event.cc
+++ b/src/trace_redaction/redact_ftrace_event.cc
@@ -30,68 +30,76 @@
base::Status RedactFtraceEvent::Transform(const Context& context,
std::string* packet) const {
- protozero::ConstBytes packet_bytes = {
- reinterpret_cast<const uint8_t*>(packet->data()), packet->size()};
-
protozero::HeapBuffered<protos::pbzero::TracePacket> message;
- RedactPacket(context, packet_bytes, message.get());
+ protozero::ProtoDecoder decoder(*packet);
+
+ // Treat FtraceEvents (bundle) as a special case.
+ for (auto f = decoder.ReadField(); f.valid(); f = decoder.ReadField()) {
+ if (f.id() == protos::pbzero::TracePacket::kFtraceEventsFieldNumber) {
+ RedactEvents(context, f, message->set_ftrace_events());
+ } else {
+ proto_util::AppendField(f, message.get());
+ }
+ }
+
packet->assign(message.SerializeAsString());
return base::OkStatus();
}
-// Iterate over every field in a packet, treating FtraceEvents (bundle) as a
-// special case.
-void RedactFtraceEvent::RedactPacket(
- const Context& context,
- protozero::ConstBytes bytes,
- protos::pbzero::TracePacket* message) const {
- protozero::ProtoDecoder decoder(bytes);
-
- for (auto field = decoder.ReadField(); field.valid();
- field = decoder.ReadField()) {
- if (field.id() == protos::pbzero::TracePacket::kFtraceEventsFieldNumber) {
- RedactEvents(context, field.as_bytes(), message->set_ftrace_events());
- } else {
- proto_util::AppendField(field, message);
- }
- }
-}
-
// Iterate over every field in FtraceEvents (bundle), treating FtraceEvent as a
// special case (calls the correct redaction).
void RedactFtraceEvent::RedactEvents(
const Context& context,
- protozero::ConstBytes bytes,
+ protozero::Field bundle,
protos::pbzero::FtraceEventBundle* message) const {
- protozero::ProtoDecoder decoder(bytes);
+ PERFETTO_DCHECK(bundle.id() ==
+ protos::pbzero::TracePacket::kFtraceEventsFieldNumber);
- for (auto field = decoder.ReadField(); field.valid();
- field = decoder.ReadField()) {
- if (field.id() == protos::pbzero::FtraceEventBundle::kEventFieldNumber) {
- RedactEvent(context, field.as_bytes(), message->add_event());
+ // There is only one bundle per packet, so creating the bundle decoder is an
+ // "okay" expense.
+ protos::pbzero::FtraceEventBundle::Decoder bundle_decoder(bundle.as_bytes());
+
+ // Even through we have `bundle_decoder` create a simpler decoder to iterate
+ // over every field.
+ protozero::ProtoDecoder decoder(bundle.as_bytes());
+
+ // Treat FtraceEvent as a special case.
+ for (auto f = decoder.ReadField(); f.valid(); f = decoder.ReadField()) {
+ if (f.id() == protos::pbzero::FtraceEventBundle::kEventFieldNumber) {
+ RedactEvent(context, bundle_decoder, f, message->add_event());
} else {
- proto_util::AppendField(field, message);
+ proto_util::AppendField(f, message);
}
}
}
void RedactFtraceEvent::RedactEvent(
const Context& context,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::Field event,
protos::pbzero::FtraceEvent* message) const {
- protozero::ProtoDecoder event(bytes);
+ PERFETTO_DCHECK(event.id() ==
+ protos::pbzero::FtraceEventBundle::kEventFieldNumber);
- for (auto field = event.ReadField(); field.valid();
- field = event.ReadField()) {
- auto* mod = redactions_.Find(field.id());
+ // A modifier can/will change the decoder by calling ReadField(). To avoid a
+ // modifier from interfering with the this function's loop, a reusable decoder
+ // is used for each modifier call.
+ protozero::ProtoDecoder outer_decoder(event.as_bytes());
+ protozero::ProtoDecoder inner_decoder(event.as_bytes());
+ // If there is a handler for a field, treat it as a special case.
+ for (auto f = outer_decoder.ReadField(); f.valid();
+ f = outer_decoder.ReadField()) {
+ auto* mod = redactions_.Find(f.id());
if (mod && mod->get()) {
- protos::pbzero::FtraceEvent::Decoder event_decoder(bytes);
- mod->get()->Redact(context, event_decoder, field.as_bytes(), message);
+ // Reset the decoder so that it appears like a "new" decoder to the
+ // modifier.
+ inner_decoder.Reset();
+ mod->get()->Redact(context, bundle, inner_decoder, message);
} else {
- proto_util::AppendField(field, message);
+ proto_util::AppendField(f, message);
}
}
}
diff --git a/src/trace_redaction/redact_ftrace_event.h b/src/trace_redaction/redact_ftrace_event.h
index fe282cf..9de6cc6 100644
--- a/src/trace_redaction/redact_ftrace_event.h
+++ b/src/trace_redaction/redact_ftrace_event.h
@@ -18,11 +18,16 @@
#define SRC_TRACE_REDACTION_REDACT_FTRACE_EVENT_H_
#include <cstdint>
+#include <memory>
+#include <string>
+#include "perfetto/base/status.h"
#include "perfetto/ext/base/flat_hash_map.h"
+#include "perfetto/protozero/field.h"
#include "src/trace_redaction/trace_redaction_framework.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
+#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.pbzero.h"
namespace perfetto::trace_redaction {
@@ -33,11 +38,14 @@
public:
virtual ~FtraceEventRedaction();
- // Write a new version of the event to the message.
+ // Write at most one field from `event` to `event_message`. This relies on the
+ // honor system; other redactions may be registered on other values.
+ //
+ // - event: effectively "protos::pbzero::FtraceEvent::Decoder"
virtual base::Status Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const = 0;
};
@@ -46,23 +54,21 @@
base::Status Transform(const Context& context,
std::string* packet) const override;
- // Add a new redaction. T must extend FtraceEventRedaction.
- template <uint32_t field_id, typename T>
+ // Add a new redaction. T must extend FtraceEventRedaction. This relies on the
+ // honor system; no more than one redaction can be mapped to a field.
+ template <uint32_t field_id, class T>
void emplace_back() {
redactions_.Insert(field_id, std::make_unique<T>());
}
private:
- void RedactPacket(const Context& context,
- protozero::ConstBytes bytes,
- protos::pbzero::TracePacket* message) const;
-
void RedactEvents(const Context& context,
- protozero::ConstBytes bytes,
+ protozero::Field bundle,
protos::pbzero::FtraceEventBundle* message) const;
void RedactEvent(const Context& context,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::Field event,
protos::pbzero::FtraceEvent* message) const;
base::FlatHashMap<uint32_t, std::unique_ptr<FtraceEventRedaction>>
diff --git a/src/trace_redaction/redact_process_free.cc b/src/trace_redaction/redact_process_free.cc
index 7294d68..8f5a83e 100644
--- a/src/trace_redaction/redact_process_free.cc
+++ b/src/trace_redaction/redact_process_free.cc
@@ -44,20 +44,19 @@
// of this, the timeline is not needed.
base::Status RedactProcessFree::Redact(
const Context&,
- const protos::pbzero::FtraceEvent::Decoder&,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder&,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const {
- // SchedProcessFreeFtraceEvent
- protozero::ProtoDecoder process_free_decoder(bytes);
-
- // There must be pid. If there's no pid, the safest option is to drop it.
- auto pid = process_free_decoder.FindField(
- protos::pbzero::SchedProcessFreeFtraceEvent::kPidFieldNumber);
-
- if (!pid.valid()) {
- return base::OkStatus();
+ auto sched_process_free = event.FindField(
+ protos::pbzero::FtraceEvent::kSchedProcessFreeFieldNumber);
+ if (!sched_process_free.valid()) {
+ return base::ErrStatus(
+ "RedactProcessFree: was used for unsupported field type");
}
+ // SchedProcessFreeFtraceEvent
+ protozero::ProtoDecoder process_free_decoder(sched_process_free.as_bytes());
+
auto* process_free_message = event_message->set_sched_process_free();
// Replace the comm with an empty string instead of dropping the comm field.
diff --git a/src/trace_redaction/redact_process_free.h b/src/trace_redaction/redact_process_free.h
index 8d24153..19e954c 100644
--- a/src/trace_redaction/redact_process_free.h
+++ b/src/trace_redaction/redact_process_free.h
@@ -31,8 +31,8 @@
base::Status Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const override;
};
diff --git a/src/trace_redaction/redact_process_free_unittest.cc b/src/trace_redaction/redact_process_free_unittest.cc
index ee119ca..04064ae 100644
--- a/src/trace_redaction/redact_process_free_unittest.cc
+++ b/src/trace_redaction/redact_process_free_unittest.cc
@@ -29,66 +29,72 @@
namespace perfetto::trace_redaction {
-class RedactProcessFreeTest : public testing::Test {};
+class RedactProcessFreeTest : public testing::Test {
+ protected:
+ void SetUp() override {
+ auto* source_event = bundle.add_event();
+ source_event->set_timestamp(123456789);
+ source_event->set_pid(10);
+ }
-TEST_F(RedactProcessFreeTest, ClearsComm) {
- protos::gen::FtraceEvent source_event;
- source_event.set_timestamp(123456789);
- source_event.set_pid(10);
+ base::Status Redact(protos::pbzero::FtraceEvent* event_message) {
+ RedactProcessFree redact;
+ Context context;
- auto* process_free = source_event.mutable_sched_process_free();
+ auto bundle_str = bundle.SerializeAsString();
+ protos::pbzero::FtraceEventBundle::Decoder bundle_decoder(bundle_str);
+
+ auto event_str = bundle.event().back().SerializeAsString();
+ protos::pbzero::FtraceEvent::Decoder event_decoder(event_str);
+
+ return redact.Redact(context, bundle_decoder, event_decoder, event_message);
+ }
+
+ protos::gen::FtraceEventBundle bundle;
+};
+
+// A free event will always test as "not active". So the comm value should
+// always be replaced with an empty string.
+TEST_F(RedactProcessFreeTest, ClearsCommValue) {
+ auto* process_free =
+ bundle.mutable_event()->back().mutable_sched_process_free();
process_free->set_comm("comm-a");
process_free->set_pid(11);
- RedactProcessFree redact;
- Context context;
-
- protos::pbzero::FtraceEvent::Decoder event_decoder(
- source_event.SerializeAsString());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(event_message.get());
ASSERT_OK(result) << result.c_message();
protos::gen::FtraceEvent redacted_event;
redacted_event.ParseFromString(event_message.SerializeAsString());
// No process free event should have been added to the ftrace event.
- ASSERT_FALSE(redacted_event.has_sched_process_free());
+ ASSERT_TRUE(redacted_event.has_sched_process_free());
+ ASSERT_TRUE(redacted_event.sched_process_free().has_comm());
+ ASSERT_TRUE(redacted_event.sched_process_free().comm().empty());
}
-// Even if there is no pid in the process free event, the process free event
-// should still exist but no comm value should be present.
+// Even if there is no pid in the process free event, the comm value should be
+// replaced with an empty string.
TEST_F(RedactProcessFreeTest, NoPidClearsEvent) {
- protos::gen::FtraceEvent source_event;
- source_event.set_timestamp(123456789);
- source_event.set_pid(10);
-
- // Don't add a pid. This should stop the process free event from being added
- // to the event message.
- auto* process_free = source_event.mutable_sched_process_free();
+ // Don't add a pid. This should have no change in behaviour.
+ auto* process_free =
+ bundle.mutable_event()->back().mutable_sched_process_free();
process_free->set_comm("comm-a");
- RedactProcessFree redact;
- Context context;
-
- protos::pbzero::FtraceEvent::Decoder event_decoder(
- source_event.SerializeAsString());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- // Even if the process free event has been dropped, there should be no
- // resulting error.
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(event_message.get());
ASSERT_OK(result) << result.c_message();
protos::gen::FtraceEvent redacted_event;
redacted_event.ParseFromString(event_message.SerializeAsString());
- ASSERT_FALSE(redacted_event.has_sched_process_free());
+ // No process free event should have been added to the ftrace event.
+ ASSERT_TRUE(redacted_event.has_sched_process_free());
+ ASSERT_TRUE(redacted_event.sched_process_free().has_comm());
+ ASSERT_TRUE(redacted_event.sched_process_free().comm().empty());
}
} // namespace perfetto::trace_redaction
diff --git a/src/trace_redaction/redact_sched_switch.cc b/src/trace_redaction/redact_sched_switch.cc
index 361993d..8dca030 100644
--- a/src/trace_redaction/redact_sched_switch.cc
+++ b/src/trace_redaction/redact_sched_switch.cc
@@ -24,6 +24,20 @@
namespace perfetto::trace_redaction {
+namespace {
+
+protozero::ConstChars SanitizeCommValue(const Context& context,
+ ProcessThreadTimeline::Slice slice,
+ protozero::Field field) {
+ if (NormalizeUid(slice.uid) == NormalizeUid(context.package_uid.value())) {
+ return field.as_string();
+ }
+
+ return {};
+}
+
+} // namespace
+
// Redact sched switch trace events in an ftrace event bundle:
//
// event {
@@ -49,8 +63,8 @@
base::Status RedactSchedSwitch::Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder&,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const {
if (!context.package_uid.has_value()) {
return base::ErrStatus("RedactSchedSwitch: missing package uid");
@@ -60,11 +74,31 @@
return base::ErrStatus("RedactSchedSwitch: missing timeline");
}
- protos::pbzero::SchedSwitchFtraceEvent::Decoder sched_switch(bytes);
+ // The timestamp is needed to do the timeline look-up. If the packet has no
+ // timestamp, don't add the sched switch event. This is the safest option.
+ auto timestamp =
+ event.FindField(protos::pbzero::FtraceEvent::kTimestampFieldNumber);
+ if (!timestamp.valid()) {
+ return base::OkStatus();
+ }
+
+ auto sched_switch =
+ event.FindField(protos::pbzero::FtraceEvent::kSchedSwitchFieldNumber);
+ if (!sched_switch.valid()) {
+ return base::ErrStatus(
+ "RedactSchedSwitch: was used for unsupported field type");
+ }
+
+ protozero::ProtoDecoder sched_switch_decoder(sched_switch.as_bytes());
+
+ auto prev_pid = sched_switch_decoder.FindField(
+ protos::pbzero::SchedSwitchFtraceEvent::kPrevPidFieldNumber);
+ auto next_pid = sched_switch_decoder.FindField(
+ protos::pbzero::SchedSwitchFtraceEvent::kNextPidFieldNumber);
// There must be a prev pid and a next pid. Otherwise, the event is invalid.
// Dropping the event is the safest option.
- if (!sched_switch.has_prev_pid() || !sched_switch.has_next_pid()) {
+ if (!prev_pid.valid() || !next_pid.valid()) {
return base::OkStatus();
}
@@ -72,30 +106,21 @@
auto sched_switch_message = event_message->set_sched_switch();
auto prev_slice =
- context.timeline->Search(event.timestamp(), sched_switch.prev_pid());
+ context.timeline->Search(timestamp.as_uint64(), prev_pid.as_int32());
auto next_slice =
- context.timeline->Search(event.timestamp(), sched_switch.next_pid());
+ context.timeline->Search(timestamp.as_uint64(), next_pid.as_int32());
- // To read the fields, move the read head back to the start.
- sched_switch.Reset();
-
- for (auto field = sched_switch.ReadField(); field.valid();
- field = sched_switch.ReadField()) {
+ for (auto field = sched_switch_decoder.ReadField(); field.valid();
+ field = sched_switch_decoder.ReadField()) {
switch (field.id()) {
case protos::pbzero::SchedSwitchFtraceEvent::kNextCommFieldNumber:
- if (next_slice.uid == context.package_uid) {
- sched_switch_message->set_next_comm(field.as_string());
- } else {
- sched_switch_message->set_next_comm("");
- }
+ sched_switch_message->set_next_comm(
+ SanitizeCommValue(context, next_slice, field));
break;
case protos::pbzero::SchedSwitchFtraceEvent::kPrevCommFieldNumber:
- if (prev_slice.uid == context.package_uid) {
- sched_switch_message->set_prev_comm(field.as_string());
- } else {
- sched_switch_message->set_prev_comm("");
- }
+ sched_switch_message->set_prev_comm(
+ SanitizeCommValue(context, prev_slice, field));
break;
default:
diff --git a/src/trace_redaction/redact_sched_switch.h b/src/trace_redaction/redact_sched_switch.h
index e9d70cb..bcfa30d 100644
--- a/src/trace_redaction/redact_sched_switch.h
+++ b/src/trace_redaction/redact_sched_switch.h
@@ -31,8 +31,8 @@
base::Status Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const override;
};
diff --git a/src/trace_redaction/redact_sched_switch_unittest.cc b/src/trace_redaction/redact_sched_switch_unittest.cc
index f6a4c13..72b65f4 100644
--- a/src/trace_redaction/redact_sched_switch_unittest.cc
+++ b/src/trace_redaction/redact_sched_switch_unittest.cc
@@ -22,7 +22,6 @@
#include "protos/perfetto/trace/ftrace/ftrace_event.gen.h"
#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.gen.h"
#include "protos/perfetto/trace/ftrace/sched.gen.h"
-#include "protos/perfetto/trace/ftrace/sched.pbzero.h"
#include "protos/perfetto/trace/trace.gen.h"
#include "protos/perfetto/trace/trace_packet.gen.h"
@@ -46,36 +45,54 @@
class RedactSchedSwitchTest : public testing::Test {
protected:
void SetUp() override {
- timeline_ = std::make_unique<ProcessThreadTimeline>();
- timeline_->Append(
- ProcessThreadTimeline::Event::Open(0, kPidA, kNoParent, kUidA));
- timeline_->Append(
- ProcessThreadTimeline::Event::Open(0, kPidB, kNoParent, kUidB));
- timeline_->Sort();
+ auto* event = bundle_.add_event();
- protozero::HeapBuffered<protos::pbzero::FtraceEvent> event;
event->set_timestamp(123456789);
event->set_pid(kPidA);
- auto* sched_switch = event->set_sched_switch();
- sched_switch->set_prev_comm(kCommA.data(), kCommA.size());
+ auto* sched_switch = event->mutable_sched_switch();
+ sched_switch->set_prev_comm(std::string(kCommA));
sched_switch->set_prev_pid(kPidA);
- sched_switch->set_next_comm(kCommB.data(), kCommB.size());
+ sched_switch->set_next_comm(std::string(kCommB));
sched_switch->set_next_pid(kPidB);
+ }
- event_string_ = event.SerializeAsString();
+ base::Status Redact(const Context& context,
+ protos::pbzero::FtraceEvent* event_message) {
+ RedactSchedSwitch redact;
+
+ auto bundle_str = bundle_.SerializeAsString();
+ protos::pbzero::FtraceEventBundle::Decoder bundle_decoder(bundle_str);
+
+ auto event_str = bundle_.event().back().SerializeAsString();
+ protos::pbzero::FtraceEvent::Decoder event_decoder(event_str);
+
+ return redact.Redact(context, bundle_decoder, event_decoder, event_message);
}
const std::string& event_string() const { return event_string_; }
- std::unique_ptr<ProcessThreadTimeline> timeline() {
- return std::move(timeline_);
+ // This test breaks the rules for task_newtask and the timeline. The
+ // timeline will report the task existing before the new task event. This
+ // should not happen in the field, but it makes the test more robust.
+ std::unique_ptr<ProcessThreadTimeline> CreatePopulatedTimeline() {
+ auto timeline = std::make_unique<ProcessThreadTimeline>();
+
+ timeline->Append(
+ ProcessThreadTimeline::Event::Open(0, kPidA, kNoParent, kUidA));
+ timeline->Append(
+ ProcessThreadTimeline::Event::Open(0, kPidB, kNoParent, kUidB));
+ timeline->Sort();
+
+ return timeline;
}
private:
std::string event_string_;
std::unique_ptr<ProcessThreadTimeline> timeline_;
+
+ protos::gen::FtraceEventBundle bundle_;
};
TEST_F(RedactSchedSwitchTest, RejectMissingPackageUid) {
@@ -84,12 +101,8 @@
Context context;
context.timeline = std::make_unique<ProcessThreadTimeline>();
- protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
-
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_FALSE(result.ok());
}
@@ -99,12 +112,8 @@
Context context;
context.package_uid = kUidA;
- protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
-
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_FALSE(result.ok());
}
@@ -112,18 +121,14 @@
RedactSchedSwitch redact;
Context context;
- context.timeline = timeline();
+ context.timeline = CreatePopulatedTimeline();
// Neither pid is connected to the target package (see timeline
// initialization).
context.package_uid = kUidC;
- protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
-
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_OK(result) << result.c_message();
protos::gen::FtraceEvent event;
@@ -143,18 +148,15 @@
RedactSchedSwitch redact;
Context context;
- context.timeline = timeline();
+ context.timeline = CreatePopulatedTimeline();
// Only next pid is connected to the target package (see timeline
// initialization).
context.package_uid = kUidB;
- protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
+ auto result = Redact(context, event_message.get());
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
ASSERT_OK(result) << result.c_message();
protos::gen::FtraceEvent event;
@@ -174,18 +176,14 @@
RedactSchedSwitch redact;
Context context;
- context.timeline = timeline();
+ context.timeline = CreatePopulatedTimeline();
// Only prev pid is connected to the target package (see timeline
// initialization).
context.package_uid = kUidA;
- protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
-
- auto result =
- redact.Redact(context, event_decoder, event_decoder.sched_switch(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_OK(result) << result.c_message();
protos::gen::FtraceEvent event;
diff --git a/src/trace_redaction/redact_task_newtask.cc b/src/trace_redaction/redact_task_newtask.cc
index cb6a41c..c942612 100644
--- a/src/trace_redaction/redact_task_newtask.cc
+++ b/src/trace_redaction/redact_task_newtask.cc
@@ -16,8 +16,6 @@
#include "src/trace_redaction/redact_task_newtask.h"
-#include <string>
-
#include "src/trace_redaction/proto_util.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
@@ -26,6 +24,20 @@
namespace perfetto::trace_redaction {
+namespace {
+
+protozero::ConstChars SanitizeCommValue(const Context& context,
+ ProcessThreadTimeline::Slice slice,
+ protozero::Field field) {
+ if (NormalizeUid(slice.uid) == NormalizeUid(context.package_uid.value())) {
+ return field.as_string();
+ }
+
+ return {};
+}
+
+} // namespace
+
// Redact sched switch trace events in an ftrace event bundle:
//
// event {
@@ -43,8 +55,8 @@
// equal to "event.task_newtask.pid" (a thread cannot start itself).
base::Status RedactTaskNewTask::Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder&,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const {
if (!context.package_uid.has_value()) {
return base::ErrStatus("RedactTaskNewTask: missing package uid");
@@ -54,14 +66,24 @@
return base::ErrStatus("RedactTaskNewTask: missing timeline");
}
- // There must be a pid. If not, the message is meaningless and can be dropped.
- if (!event.has_timestamp()) {
+ // The timestamp is needed to do the timeline look-up. If the packet has no
+ // timestamp, don't add the sched switch event. This is the safest option.
+ auto timestamp =
+ event.FindField(protos::pbzero::FtraceEvent::kTimestampFieldNumber);
+ if (!timestamp.valid()) {
return base::OkStatus();
}
- protozero::ProtoDecoder new_task(bytes);
+ auto new_task =
+ event.FindField(protos::pbzero::FtraceEvent::kTaskNewtaskFieldNumber);
+ if (!new_task.valid()) {
+ return base::ErrStatus(
+ "RedactTaskNewTask: was used for unsupported field type");
+ }
- auto pid = new_task.FindField(
+ protozero::ProtoDecoder new_task_decoder(new_task.as_bytes());
+
+ auto pid = new_task_decoder.FindField(
protos::pbzero::TaskNewtaskFtraceEvent::kPidFieldNumber);
if (!pid.valid()) {
@@ -71,20 +93,16 @@
// Avoid making the message until we know that we have prev and next pids.
auto* new_task_message = event_message->set_task_newtask();
- auto slice = context.timeline->Search(event.timestamp(), pid.as_int32());
+ auto slice = context.timeline->Search(timestamp.as_uint64(), pid.as_int32());
- for (auto field = new_task.ReadField(); field.valid();
- field = new_task.ReadField()) {
+ for (auto field = new_task_decoder.ReadField(); field.valid();
+ field = new_task_decoder.ReadField()) {
+ // Perfetto view (ui.perfetto.dev) crashes if the comm value is missing.
+ // To work around this, the comm value is replaced with an empty string.
+ // This appears to work.
if (field.id() ==
protos::pbzero::TaskNewtaskFtraceEvent::kCommFieldNumber) {
- if (slice.uid == context.package_uid) {
- proto_util::AppendField(field, new_task_message);
- } else {
- // Perfetto view (ui.perfetto.dev) crashes if the comm value is missing.
- // To work around this, the comm value is replaced with an empty string.
- // This appears to work.
- new_task_message->set_comm("");
- }
+ new_task_message->set_comm(SanitizeCommValue(context, slice, field));
} else {
proto_util::AppendField(field, new_task_message);
}
diff --git a/src/trace_redaction/redact_task_newtask.h b/src/trace_redaction/redact_task_newtask.h
index 51d384c..51c1124 100644
--- a/src/trace_redaction/redact_task_newtask.h
+++ b/src/trace_redaction/redact_task_newtask.h
@@ -31,8 +31,8 @@
base::Status Redact(
const Context& context,
- const protos::pbzero::FtraceEvent::Decoder& event,
- protozero::ConstBytes bytes,
+ const protos::pbzero::FtraceEventBundle::Decoder& bundle,
+ protozero::ProtoDecoder& event,
protos::pbzero::FtraceEvent* event_message) const override;
};
diff --git a/src/trace_redaction/redact_task_newtask_unittest.cc b/src/trace_redaction/redact_task_newtask_unittest.cc
index d09f044..33886f6 100644
--- a/src/trace_redaction/redact_task_newtask_unittest.cc
+++ b/src/trace_redaction/redact_task_newtask_unittest.cc
@@ -16,11 +16,15 @@
#include "src/trace_redaction/redact_task_newtask.h"
#include "perfetto/protozero/scattered_heap_buffer.h"
+#include "protos/perfetto/trace/ftrace/task.gen.h"
#include "test/gtest_and_gmock.h"
#include "protos/perfetto/trace/ftrace/ftrace_event.gen.h"
-#include "protos/perfetto/trace/ftrace/task.gen.h"
-#include "protos/perfetto/trace/ftrace/task.pbzero.h"
+#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
+#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.gen.h"
+#include "protos/perfetto/trace/ftrace/sched.gen.h"
+#include "protos/perfetto/trace/trace.gen.h"
+#include "protos/perfetto/trace/trace_packet.gen.h"
namespace perfetto::trace_redaction {
@@ -40,37 +44,52 @@
class RedactTaskNewTaskTest : public testing::Test {
protected:
void SetUp() override {
- timeline_ = std::make_unique<ProcessThreadTimeline>();
- timeline_->Append(
- ProcessThreadTimeline::Event::Open(0, kPidA, kNoParent, kUidA));
- timeline_->Append(
- ProcessThreadTimeline::Event::Open(0, kPidB, kNoParent, kUidB));
- timeline_->Sort();
+ auto* event = bundle_.add_event();
- // This test breaks the rules for task_newtask and the timeline. The
- // timeline will report the task existing before the new task event. This
- // should not happen in the field, but it makes the test more robust.
- protozero::HeapBuffered<protos::pbzero::FtraceEvent> event;
event->set_timestamp(123456789);
event->set_pid(kPidA);
- auto* new_task = event->set_task_newtask();
- new_task->set_comm(kCommA.data(), kCommA.size());
+ auto* new_task = event->mutable_task_newtask();
+ new_task->set_comm(std::string(kCommA));
new_task->set_pid(kPidA);
+ }
- event_string_ = event.SerializeAsString();
+ base::Status Redact(const Context& context,
+ protos::pbzero::FtraceEvent* event_message) {
+ RedactTaskNewTask redact;
+
+ auto bundle_str = bundle_.SerializeAsString();
+ protos::pbzero::FtraceEventBundle::Decoder bundle_decoder(bundle_str);
+
+ auto event_str = bundle_.event().back().SerializeAsString();
+ protos::pbzero::FtraceEvent::Decoder event_decoder(event_str);
+
+ return redact.Redact(context, bundle_decoder, event_decoder, event_message);
}
const std::string& event_string() const { return event_string_; }
- std::unique_ptr<ProcessThreadTimeline> timeline() {
- return std::move(timeline_);
+ // This test breaks the rules for task_newtask and the timeline. The
+ // timeline will report the task existing before the new task event. This
+ // should not happen in the field, but it makes the test more robust.
+ std::unique_ptr<ProcessThreadTimeline> CreatePopulatedTimeline() {
+ auto timeline = std::make_unique<ProcessThreadTimeline>();
+
+ timeline->Append(
+ ProcessThreadTimeline::Event::Open(0, kPidA, kNoParent, kUidA));
+ timeline->Append(
+ ProcessThreadTimeline::Event::Open(0, kPidB, kNoParent, kUidB));
+ timeline->Sort();
+
+ return timeline;
}
private:
std::string event_string_;
std::unique_ptr<ProcessThreadTimeline> timeline_;
+
+ protos::gen::FtraceEventBundle bundle_;
};
TEST_F(RedactTaskNewTaskTest, RejectMissingPackageUid) {
@@ -82,9 +101,7 @@
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result =
- redact.Redact(context, event_decoder, event_decoder.task_newtask(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_FALSE(result.ok());
}
@@ -97,9 +114,7 @@
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result =
- redact.Redact(context, event_decoder, event_decoder.task_newtask(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_FALSE(result.ok());
}
@@ -110,14 +125,12 @@
// keep its comm value.
Context context;
context.package_uid = kUidA;
- context.timeline = timeline();
+ context.timeline = CreatePopulatedTimeline();
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result =
- redact.Redact(context, event_decoder, event_decoder.task_newtask(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_TRUE(result.ok());
protos::gen::FtraceEvent redacted_event;
@@ -135,14 +148,12 @@
// lose its comm value.
Context context;
context.package_uid = kUidB;
- context.timeline = timeline();
+ context.timeline = CreatePopulatedTimeline();
protos::pbzero::FtraceEvent::Decoder event_decoder(event_string());
protozero::HeapBuffered<protos::pbzero::FtraceEvent> event_message;
- auto result =
- redact.Redact(context, event_decoder, event_decoder.task_newtask(),
- event_message.get());
+ auto result = Redact(context, event_message.get());
ASSERT_TRUE(result.ok());
protos::gen::FtraceEvent redacted_event;
