Split packets in IPC-friendly-sized slices after filtering
Client side filtering creates reassembles the packet into one big
slice. If the slice is too large (larger than ipc::kIPCBufferSize), the
packet will hit an assertion later on.
Changing the MessageFilter to avoid creating big slices would be a nicer
fix. We should probably do that in the future.
Bug: 195065199
Change-Id: Id6323e8a64d4ff71d40ec902717069153b2cd23d
diff --git a/Android.bp b/Android.bp
index 85087bb..46db7fe 100644
--- a/Android.bp
+++ b/Android.bp
@@ -1074,6 +1074,7 @@
":perfetto_src_ipc_perfetto_ipc",
":perfetto_src_kallsyms_kallsyms",
":perfetto_src_protozero_filtering_bytecode_common",
+ ":perfetto_src_protozero_filtering_bytecode_generator",
":perfetto_src_protozero_filtering_bytecode_parser",
":perfetto_src_protozero_filtering_message_filter",
":perfetto_src_protozero_protozero",
@@ -1739,6 +1740,7 @@
":perfetto_src_profiling_memory_scoped_spinlock",
":perfetto_src_profiling_memory_wire_protocol",
":perfetto_src_protozero_filtering_bytecode_common",
+ ":perfetto_src_protozero_filtering_bytecode_generator",
":perfetto_src_protozero_filtering_bytecode_parser",
":perfetto_src_protozero_filtering_message_filter",
":perfetto_src_protozero_protozero",
diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc
index 9ca22d8..114000d 100644
--- a/src/tracing/core/tracing_service_impl.cc
+++ b/src/tracing/core/tracing_service_impl.cc
@@ -287,6 +287,29 @@
PERFETTO_FATAL("For GCC");
}
+// Appends `data` (which has `size` bytes), to `*packet`. Splits the data in
+// slices no larger than `max_slice_size`.
+void AppendOwnedSlicesToPacket(std::unique_ptr<uint8_t[]> data,
+ size_t size,
+ size_t max_slice_size,
+ perfetto::TracePacket* packet) {
+ if (size <= max_slice_size) {
+ packet->AddSlice(Slice::TakeOwnership(std::move(data), size));
+ return;
+ }
+ uint8_t* src_ptr = data.get();
+ for (size_t size_left = size; size_left > 0;) {
+ const size_t slice_size = std::min(size_left, max_slice_size);
+
+ Slice slice = Slice::Allocate(slice_size);
+ memcpy(slice.own_data(), src_ptr, slice_size);
+ packet->AddSlice(std::move(slice));
+
+ src_ptr += slice_size;
+ size_left -= slice_size;
+ }
+}
+
} // namespace
// These constants instead are defined in the header because are used by tests.
@@ -2168,8 +2191,9 @@
continue;
}
tracing_session->filter_output_bytes += filtered_packet.size;
- it->AddSlice(Slice::TakeOwnership(std::move(filtered_packet.data),
- filtered_packet.size));
+ AppendOwnedSlicesToPacket(std::move(filtered_packet.data),
+ filtered_packet.size, kMaxTracePacketSliceSize,
+ &*it);
} // for (packet)
} // if (trace_filter)
diff --git a/src/tracing/core/tracing_service_impl.h b/src/tracing/core/tracing_service_impl.h
index 401aaa1..8c50713 100644
--- a/src/tracing/core/tracing_service_impl.h
+++ b/src/tracing/core/tracing_service_impl.h
@@ -77,6 +77,9 @@
static constexpr uint8_t kSyncMarker[] = {0x82, 0x47, 0x7a, 0x76, 0xb2, 0x8d,
0x42, 0xba, 0x81, 0xdc, 0x33, 0x32,
0x6d, 0x57, 0xa0, 0x79};
+ static constexpr size_t kMaxTracePacketSliceSize =
+ 128 * 1024 - 512; // This is ipc::kIPCBufferSize - 512, see assertion in
+ // tracing_integration_test.cc and b/195065199
// The implementation behind the service endpoint exposed to each producer.
class ProducerEndpointImpl : public TracingService::ProducerEndpoint {
diff --git a/src/tracing/test/tracing_integration_test.cc b/src/tracing/test/tracing_integration_test.cc
index 134ffd0..d1ca340 100644
--- a/src/tracing/test/tracing_integration_test.cc
+++ b/src/tracing/test/tracing_integration_test.cc
@@ -113,6 +113,11 @@
EXPECT_EQ(0u, buf_stats.abi_violations());
}
+static_assert(TracingServiceImpl::kMaxTracePacketSliceSize <=
+ ipc::kIPCBufferSize - 512,
+ "Tracing service max packet slice should be smaller than IPC "
+ "buffer size (with some headroom)");
+
} // namespace
class TracingIntegrationTest : public ::testing::Test {
diff --git a/test/BUILD.gn b/test/BUILD.gn
index 64e683a..01ae3c6 100644
--- a/test/BUILD.gn
+++ b/test/BUILD.gn
@@ -41,6 +41,7 @@
]
if (enable_perfetto_traced_probes) {
deps += [
+ "../src/protozero/filtering:bytecode_generator",
"../src/traced/probes/ftrace",
"../src/traced/probes/ftrace:ftrace_procfs",
]
diff --git a/test/end_to_end_integrationtest.cc b/test/end_to_end_integrationtest.cc
index 75cfa88..1ec13d3 100644
--- a/test/end_to_end_integrationtest.cc
+++ b/test/end_to_end_integrationtest.cc
@@ -41,6 +41,7 @@
#include "perfetto/tracing/core/tracing_service_state.h"
#include "src/base/test/test_task_runner.h"
#include "src/base/test/utils.h"
+#include "src/protozero/filtering/filter_bytecode_generator.h"
#include "src/traced/probes/ftrace/ftrace_controller.h"
#include "src/traced/probes/ftrace/ftrace_procfs.h"
#include "test/gtest_and_gmock.h"
@@ -74,8 +75,11 @@
namespace {
using ::testing::ContainsRegex;
+using ::testing::Each;
using ::testing::ElementsAreArray;
using ::testing::HasSubstr;
+using ::testing::Property;
+using ::testing::SizeIs;
std::string RandomTraceFileName() {
#if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID)
@@ -1125,6 +1129,67 @@
}
}
+// Regression test for b/195065199. Check that trace filtering works when a
+// packet size exceeds the IPC limit. This tests that the tracing service, when
+// reassembling packets after filtering, doesn't "overglue" them. They still
+// need to be slice-able to fit into the ReadBuffers ipc.
+TEST_F(PerfettoTest, TraceFilterLargePackets) {
+ base::TestTaskRunner task_runner;
+ TestHelper helper(&task_runner);
+
+ helper.StartServiceIfRequired();
+ helper.ConnectFakeProducer();
+ helper.ConnectConsumer();
+ helper.WaitForConsumerConnect();
+
+ TraceConfig trace_config;
+ trace_config.add_buffers()->set_size_kb(1024 * 16);
+ trace_config.set_duration_ms(500);
+ auto* prod_config = trace_config.add_producers();
+ prod_config->set_producer_name("android.perfetto.FakeProducer");
+ prod_config->set_shm_size_kb(1024 * 16);
+ prod_config->set_page_size_kb(32);
+
+ static constexpr size_t kNumPackets = 3;
+ static constexpr uint32_t kRandomSeed = 42;
+ static constexpr uint32_t kMsgSize = 8 * ipc::kIPCBufferSize;
+ auto* ds_config = trace_config.add_data_sources()->mutable_config();
+ ds_config->set_name("android.perfetto.FakeProducer");
+ auto* test_config = ds_config->mutable_for_testing();
+ test_config->set_seed(kRandomSeed);
+ test_config->set_message_count(kNumPackets);
+ test_config->set_message_size(kMsgSize);
+ test_config->set_send_batch_on_register(true);
+
+ protozero::FilterBytecodeGenerator filt;
+ // Message 0: root Trace proto.
+ filt.AddNestedField(1 /* root trace.packet*/, 1);
+ filt.EndMessage();
+
+ // Message 1: TracePacket proto. Allow all fields.
+ filt.AddSimpleFieldRange(1, 1000);
+ filt.EndMessage();
+
+ trace_config.mutable_trace_filter()->set_bytecode(filt.Serialize());
+
+ // The data source is configured to emit another batch when it is started via
+ // send_batch_on_register in the TestConfig.
+ helper.StartTracing(trace_config);
+ helper.WaitForTracingDisabled();
+
+ helper.ReadData();
+ helper.WaitForReadData(/* read_count */ 0, /* timeout_ms */ 10000);
+
+ const std::vector<protos::gen::TracePacket>& packets = helper.trace();
+ EXPECT_EQ(packets.size(), kNumPackets);
+ EXPECT_THAT(packets,
+ Each(Property(&protos::gen::TracePacket::has_for_testing, true)));
+ EXPECT_THAT(
+ packets,
+ Each(Property(&protos::gen::TracePacket::for_testing,
+ Property(&protos::gen::TestEvent::str, SizeIs(kMsgSize)))));
+}
+
// Disable cmdline tests on sanitizets because they use fork() and that messes
// up leak / races detections, which has been fixed only recently (see
// https://github.com/google/sanitizers/issues/836 ).
