Accept non zero-terminated strings in ftrace fields
ReadIntoString() fails if strings are not null terminated.
There are three cases in which ReadIntoString() is called:
1. For fixed fields declared as:
```
field:char newcomm[16]; offset:28; size:16; signed:1;
```
the max length of the string is bounded by the fixed size.
2. For variable length fields declared at the end of an event:
```
field:char buf[]; offset:16; size:0; signed:1;
```
the max length of the buffer is bounded by the event size.
3. For __data_loc fields:
```
field:__data_loc char[] name; offset:16; size:4; signed:1;
```
each field has an offset and a size. The max length of the string is
bounded by the fixed size.
In some rare cases, e.g., if this kernel commit is missing:
`f0a515780393("tracing: Don't make assumptions about length of string on task rename")`
the kernel can output a buffer that's not zero-terminated (e.g. for case
1)
In all cases, the string size is limited explicitly. The code should
still stop copying when it finds the '\0' terminator, but, if it doesn't
find that, it could still copy what's there. There's no risk of going
out of bounds, so this commit stops complaining (and eventually hitting
a PERFETTO_CHECK), if strings are not null terminated.
Bug: 205763418
Change-Id: I2159ecbc4dd42d31f54613efa5e77c9d7eca09fb
diff --git a/src/traced/probes/ftrace/cpu_reader.cc b/src/traced/probes/ftrace/cpu_reader.cc
index 6e09c01..7867ff6 100644
--- a/src/traced/probes/ftrace/cpu_reader.cc
+++ b/src/traced/probes/ftrace/cpu_reader.cc
@@ -72,18 +72,14 @@
uint32_t time_delta : 27;
};
-bool ReadIntoString(const uint8_t* start,
- const uint8_t* end,
+// Reads a string from `start` until the first '\0' byte or until fixed_len
+// characters have been read. Appends it to `*out` as field `field_id`.
+void ReadIntoString(const uint8_t* start,
+ size_t fixed_len,
uint32_t field_id,
protozero::Message* out) {
- for (const uint8_t* c = start; c < end; c++) {
- if (*c != '\0')
- continue;
- out->AppendBytes(field_id, reinterpret_cast<const char*>(start),
- static_cast<uintptr_t>(c - start));
- return true;
- }
- return false;
+ size_t len = strnlen(reinterpret_cast<const char*>(start), fixed_len);
+ out->AppendBytes(field_id, reinterpret_cast<const char*>(start), len);
}
bool ReadDataLoc(const uint8_t* start,
@@ -104,12 +100,11 @@
const uint16_t offset = data & 0xffff;
const uint16_t len = (data >> 16) & 0xffff;
const uint8_t* const string_start = start + offset;
- const uint8_t* const string_end = string_start + len;
- if (string_start <= start || string_end > end) {
+ if (string_start <= start || string_start + len > end) {
PERFETTO_DFATAL("Buffer overflowed.");
return false;
}
- ReadIntoString(string_start, string_end, field.proto_field_id, message);
+ ReadIntoString(string_start, len, field.proto_field_id, message);
return true;
}
@@ -752,12 +747,14 @@
ReadIntoVarInt<int64_t>(field_start, field_id, message);
return true;
case kFixedCStringToString:
- // TODO(hjd): Add AppendMaxLength string to protozero.
- return ReadIntoString(field_start, field_start + field.ftrace_size,
- field_id, message);
+ // TODO(hjd): Kernel-dive to check this how size:0 char fields work.
+ ReadIntoString(field_start, field.ftrace_size, field_id, message);
+ return true;
case kCStringToString:
// TODO(hjd): Kernel-dive to check this how size:0 char fields work.
- return ReadIntoString(field_start, end, field_id, message);
+ ReadIntoString(field_start, static_cast<size_t>(end - field_start),
+ field_id, message);
+ return true;
case kStringPtrToString: {
uint64_t n = 0;
// The ftrace field may be 8 or 4 bytes and we need to copy it into the
diff --git a/src/traced/probes/ftrace/cpu_reader_unittest.cc b/src/traced/probes/ftrace/cpu_reader_unittest.cc
index 84e6c6f..c918af8 100644
--- a/src/traced/probes/ftrace/cpu_reader_unittest.cc
+++ b/src/traced/probes/ftrace/cpu_reader_unittest.cc
@@ -39,6 +39,7 @@
#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.pbzero.h"
#include "protos/perfetto/trace/ftrace/power.gen.h"
#include "protos/perfetto/trace/ftrace/sched.gen.h"
+#include "protos/perfetto/trace/ftrace/task.gen.h"
#include "protos/perfetto/trace/trace_packet.gen.h"
#include "src/traced/probes/ftrace/test/test_messages.gen.h"
#include "src/traced/probes/ftrace/test/test_messages.pbzero.h"
@@ -540,20 +541,20 @@
}
// This event is as the event for ParseSinglePrint above except the string
-// is extended to overflow the page size written in the header.
-static ExamplePage g_single_print_malformed{
+// is extended and not null terminated.
+static ExamplePage g_single_print_non_null_terminated{
"synthetic",
R"(
- 00000000: ba12 6a33 c628 0200 2c00 0000 0000 0000 ................
- 00000010: def0 ec67 8d21 0000 0800 0000 0500 0001 ................
- 00000020: 2870 0000 ac5d 1661 86ff ffff 4865 6c6c ................
- 00000030: 6f2c 2077 6f72 6c64 2120 776f 726c 6421 ................
- 00000040: 0a00 ff00 0000 0000 0000 0000 0000 0000 ................
+ 00000000: ba12 6a33 c628 0200 2c00 0000 0000 0000 ..j3.(..,.......
+ 00000010: def0 ec67 8d21 0000 0800 0000 0500 0001 ...g.!..........
+ 00000020: 2870 0000 ac5d 1661 86ff ffff 4865 6c6c (p...].a....Hell
+ 00000030: 6f2c 2077 6f72 6c64 2161 6161 6161 6161 o, world!aaaaaaa
+ 00000040: 6161 6161 6161 6161 6161 6161 6161 6161 aaaaaaaaaaaaaaaa
)",
};
-TEST(CpuReaderTest, ParseSinglePrintMalformed) {
- const ExamplePage* test_case = &g_single_print_malformed;
+TEST(CpuReaderTest, ParseSinglePrintNonNullTerminated) {
+ const ExamplePage* test_case = &g_single_print_non_null_terminated;
BundleProvider bundle_provider(base::kPageSize);
ProtoTranslationTable* table = GetTable(test_case->name);
@@ -579,19 +580,68 @@
parse_pos, &page_header.value(), table, &ds_config, compact_buffer.get(),
bundle_provider.writer(), &metadata);
- ASSERT_EQ(0u, evt_bytes);
+ ASSERT_EQ(44u, evt_bytes);
auto bundle = bundle_provider.ParseProto();
ASSERT_TRUE(bundle);
ASSERT_EQ(bundle->event().size(), 1u);
- // Although one field is malformed we still see data for the rest
- // since we write the fields as we parse them for speed.
const protos::gen::FtraceEvent& event = bundle->event()[0];
EXPECT_EQ(event.pid(), 28712ul);
EXPECT_TRUE(WithinOneMicrosecond(event.timestamp(), 608934, 535199));
+ EXPECT_EQ(event.print().buf(), "Hello, world!aaa");
+}
+
+static ExamplePage g_single_print_zero_size{
+ "synthetic",
+ R"(
+ 00000000: ba12 6a33 c628 0200 2c00 0000 0000 0000 ..j3.(..,.......
+ 00000010: def0 ec67 8d21 0000 0800 0000 0500 0001 ...g.!..........
+ 00000020: 2870 0000 ac5d 1661 86ff ffff 0000 0000 (p...].a........
+ 00000030: 0000 0000 0000 0000 0000 0000 0000 0000 ................
+ 00000040: 0000 0000 0000 0000 0000 0000 0000 0000 ................
+ )",
+};
+
+TEST(CpuReaderTest, ParseSinglePrintZeroSize) {
+ const ExamplePage* test_case = &g_single_print_zero_size;
+
+ BundleProvider bundle_provider(base::kPageSize);
+ ProtoTranslationTable* table = GetTable(test_case->name);
+ auto page = PageFromXxd(test_case->data);
+
+ FtraceDataSourceConfig ds_config = EmptyConfig();
+ ds_config.event_filter.AddEnabledEvent(
+ table->EventToFtraceId(GroupAndName("ftrace", "print")));
+
+ FtraceMetadata metadata{};
+ std::unique_ptr<CompactSchedBuffer> compact_buffer(new CompactSchedBuffer());
+ const uint8_t* parse_pos = page.get();
+ base::Optional<CpuReader::PageHeader> page_header =
+ CpuReader::ParsePageHeader(&parse_pos, table->page_header_size_len());
+
+ const uint8_t* page_end = page.get() + base::kPageSize;
+ ASSERT_TRUE(page_header.has_value());
+ EXPECT_FALSE(page_header->lost_events);
+ EXPECT_TRUE(parse_pos < page_end);
+ EXPECT_TRUE(parse_pos + page_header->size < page_end);
+
+ size_t evt_bytes = CpuReader::ParsePagePayload(
+ parse_pos, &page_header.value(), table, &ds_config, compact_buffer.get(),
+ bundle_provider.writer(), &metadata);
+
+ ASSERT_EQ(44u, evt_bytes);
+
+ auto bundle = bundle_provider.ParseProto();
+ ASSERT_TRUE(bundle);
+ ASSERT_EQ(bundle->event().size(), 1u);
+ const protos::gen::FtraceEvent& event = bundle->event()[0];
+ EXPECT_EQ(event.pid(), 28712ul);
+ EXPECT_TRUE(WithinOneMicrosecond(event.timestamp(), 608934, 535199));
+ EXPECT_TRUE(event.print().has_buf());
EXPECT_EQ(event.print().buf(), "");
}
+
TEST(CpuReaderTest, FilterByEvent) {
const ExamplePage* test_case = &g_single_print;
@@ -1136,6 +1186,42 @@
EXPECT_THAT(metadata.pids, Contains(9999));
}
+// Regression test for b/205763418: Kernels without f0a515780393("tracing: Don't
+// make assumptions about length of string on task rename") can output non
+// zero-terminated strings in some cases. Even though it's a kernel bug, there's
+// no point in rejecting that.
+TEST(CpuReaderTest, EventNonZeroTerminated) {
+ BundleProvider bundle_provider(base::kPageSize);
+
+ BinaryWriter writer;
+ ProtoTranslationTable* table = GetTable("android_seed_N2F62_3.10.49");
+
+ constexpr uint32_t kTaskRenameId = 19;
+
+ writer.Write<int32_t>(1001); // Common field.
+ writer.Write<int32_t>(9999); // Common pid
+ writer.Write<int32_t>(9999); // Pid
+ writer.WriteFixedString(16, "Hello"); // Old Comm
+ std::array<char, 16> newcomm;
+ memcpy(&newcomm, "0123456789abcdef", sizeof newcomm);
+ writer.Write(newcomm); // New Comm - not null terminated
+ writer.Write<uint64_t>(10); // flags
+ writer.Write<int16_t>(10); // oom_score_adj
+
+ auto input = writer.GetCopy();
+ auto length = writer.written();
+ FtraceMetadata metadata{};
+
+ ASSERT_TRUE(CpuReader::ParseEvent(
+ kTaskRenameId, input.get(), input.get() + length, table,
+ bundle_provider.writer()->add_event(), &metadata));
+ std::unique_ptr<protos::gen::FtraceEventBundle> a =
+ bundle_provider.ParseProto();
+ ASSERT_NE(a, nullptr);
+ ASSERT_EQ(a->event().size(), 1u);
+ ASSERT_EQ(a->event()[0].task_rename().newcomm(), "0123456789abcdef");
+}
+
// Page with a single sched_switch, no data loss.
static char g_switch_page[] =
R"(
diff --git a/src/traced/probes/ftrace/proto_translation_table.cc b/src/traced/probes/ftrace/proto_translation_table.cc
index 9a69745..a644390 100644
--- a/src/traced/probes/ftrace/proto_translation_table.cc
+++ b/src/traced/probes/ftrace/proto_translation_table.cc
@@ -261,6 +261,9 @@
// are both fixed size and null terminated meaning that we can't just drop
// them directly into the protobuf (since if the string is shorter than 15
// characters we want only the bit up to the null terminator).
+ //
+ // In some rare cases (e.g. old kernel bugs) these strings might not be null
+ // terminated (b/205763418).
if (Match(type_and_name.c_str(), R"(char [a-zA-Z_]+\[[0-9]+\])")) {
*out = kFtraceFixedCString;
return true;