trace buffer: Support overriding chunks.
To support scraping incomplete chunks in the service, TraceBuffer needs
to support overriding previously copied chunks. Since chunks in the SMB
aren't ordered, scraping and thus overriding chunks may happen in
arbitrary order.
Bug: 73828976
Change-Id: I7bd55fe992453dfd88deda66c4b83c63892f86cc
diff --git a/src/tracing/core/trace_buffer.cc b/src/tracing/core/trace_buffer.cc
index a73c1bb..b67f829 100644
--- a/src/tracing/core/trace_buffer.cc
+++ b/src/tracing/core/trace_buffer.cc
@@ -107,6 +107,7 @@
ChunkID chunk_id,
uint16_t num_fragments,
uint8_t chunk_flags,
+ bool chunk_complete,
const uint8_t* src,
size_t size) {
// |record_size| = |size| + sizeof(ChunkRecord), rounded up to avoid to end
@@ -125,6 +126,102 @@
changed_since_last_read_ = true;
#endif
+ // If the chunk hasn't been completed, we should only consider the first
+ // |num_fragments - 1| packets complete. For simplicity, we simply disregard
+ // the last one when we copy the chunk.
+ if (PERFETTO_UNLIKELY(!chunk_complete)) {
+ if (num_fragments > 0) {
+ num_fragments--;
+ chunk_flags &= ~kLastPacketContinuesOnNextChunk;
+ }
+ }
+
+ ChunkRecord record(record_size);
+ record.producer_id = producer_id_trusted;
+ record.chunk_id = chunk_id;
+ record.writer_id = writer_id;
+ record.num_fragments = num_fragments;
+ record.flags = chunk_flags;
+ ChunkMeta::Key key(record);
+
+ // Check whether we have already copied the same chunk previously. This may
+ // happen if the service scrapes chunks in a potentially incomplete state
+ // before receiving commit requests for them from the producer. Note that the
+ // service may scrape and thus override chunks in arbitrary order since the
+ // chunks aren't ordered in the SMB.
+ const auto it = index_.find(key);
+ if (PERFETTO_UNLIKELY(it != index_.end())) {
+ ChunkMeta* record_meta = &it->second;
+ ChunkRecord* prev = record_meta->chunk_record;
+
+ // Verify that the old chunk's metadata corresponds to the new one.
+ // Overridden chunks should never change size, since the page layout is
+ // fixed per writer. The number of fragments should also never decrease and
+ // flags should not be removed.
+ if (PERFETTO_UNLIKELY(ChunkMeta::Key(*prev) != key ||
+ prev->size != record_size ||
+ prev->num_fragments > num_fragments ||
+ (prev->flags & chunk_flags) != prev->flags)) {
+ stats_.abi_violations++;
+ PERFETTO_DCHECK(suppress_sanity_dchecks_for_testing_);
+ return;
+ }
+
+ // If we've already started reading from chunk N+1 following this chunk N,
+ // don't override chunk N. Otherwise we may end up reading a packet from
+ // chunk N after having read from chunk N+1, thereby violating sequential
+ // read of packets. This shouldn't happen if the producer is well-behaved,
+ // because it shouldn't start chunk N+1 before completing chunk N.
+ ChunkMeta::Key subsequent_key = key;
+ static_assert(std::numeric_limits<ChunkID>::max() == kMaxChunkID,
+ "ChunkID wraps");
+ subsequent_key.chunk_id++;
+ const auto subsequent_it = index_.find(subsequent_key);
+ if (subsequent_it != index_.end() &&
+ subsequent_it->second.num_fragments_read > 0) {
+ stats_.abi_violations++;
+ PERFETTO_DCHECK(suppress_sanity_dchecks_for_testing_);
+ return;
+ }
+
+ // If this chunk was previously copied with the same number of fragments and
+ // the number didn't change, there's no need to copy it again. If the
+ // previous chunk was complete already, this should always be the case.
+ PERFETTO_DCHECK(suppress_sanity_dchecks_for_testing_ ||
+ !record_meta->is_complete ||
+ (chunk_complete && prev->num_fragments == num_fragments));
+ if (prev->num_fragments == num_fragments) {
+ TRACE_BUFFER_DLOG(" skipping recommit of identical chunk");
+ return;
+ }
+
+ // We should not have read past the last packet.
+ if (record_meta->num_fragments_read > prev->num_fragments) {
+ PERFETTO_ELOG(
+ "TraceBuffer read too many fragments from an incomplete chunk");
+ PERFETTO_DCHECK(suppress_sanity_dchecks_for_testing_);
+ return;
+ }
+
+ uint8_t* wptr = reinterpret_cast<uint8_t*>(prev);
+ TRACE_BUFFER_DLOG(" overriding chunk @ %lu, size=%zu", wptr - begin(),
+ record_size);
+
+ // Update chunk meta data stored in the index, as it may have changed.
+ record_meta->num_fragments = num_fragments;
+ record_meta->flags = chunk_flags;
+ record_meta->is_complete = chunk_complete;
+
+ // Override the ChunkRecord contents at the original |wptr|.
+ TRACE_BUFFER_DLOG(" copying @ [%lu - %lu] %zu", wptr - begin(),
+ wptr - begin() + record_size, record_size);
+ WriteChunkRecord(wptr, record, src, size);
+ TRACE_BUFFER_DLOG("Chunk raw: %s", HexDump(wptr, record_size).c_str());
+
+ // TODO(eseckler): Add a stat for rewritten chunks.
+ return;
+ }
+
// If there isn't enough room from the given write position. Write a padding
// record to clear the end of the buffer and wrap back.
const size_t cached_size_to_end = size_to_end();
@@ -137,13 +234,6 @@
PERFETTO_DCHECK(size_to_end() >= record_size);
}
- ChunkRecord record(record_size);
- record.producer_id = producer_id_trusted;
- record.chunk_id = chunk_id;
- record.writer_id = writer_id;
- record.num_fragments = num_fragments;
- record.flags = chunk_flags;
-
// At this point either |wptr_| points to an untouched part of the buffer
// (i.e. *wptr_ == 0) or we are about to overwrite one or more ChunkRecord(s).
// In the latter case we need to first figure out where the next valid
@@ -167,23 +257,15 @@
size_t padding_size = DeleteNextChunksFor(record_size);
// Now first insert the new chunk. At the end, if necessary, add the padding.
- ChunkMeta::Key key(record);
stats_.chunks_written++;
stats_.bytes_written += size;
- auto it_and_inserted =
- index_.emplace(key, ChunkMeta(GetChunkRecordAt(wptr_), num_fragments,
- chunk_flags, producer_uid_trusted));
- if (PERFETTO_UNLIKELY(!it_and_inserted.second)) {
- // More likely a producer bug, but could also be a malicious producer.
- stats_.abi_violations++;
- PERFETTO_DCHECK(suppress_sanity_dchecks_for_testing_);
- index_.erase(it_and_inserted.first);
- index_.emplace(key, ChunkMeta(GetChunkRecordAt(wptr_), num_fragments,
- chunk_flags, producer_uid_trusted));
- }
+ auto it_and_inserted = index_.emplace(
+ key, ChunkMeta(GetChunkRecordAt(wptr_), num_fragments, chunk_complete,
+ chunk_flags, producer_uid_trusted));
+ PERFETTO_DCHECK(it_and_inserted.second);
TRACE_BUFFER_DLOG(" copying @ [%lu - %lu] %zu", wptr_ - begin(),
wptr_ - begin() + record_size, record_size);
- WriteChunkRecord(record, src, size);
+ WriteChunkRecord(wptr_, record, src, size);
TRACE_BUFFER_DLOG("Chunk raw: %s", HexDump(wptr_, record_size).c_str());
wptr_ += record_size;
if (wptr_ >= end()) {
@@ -285,7 +367,7 @@
record.is_padding = 1;
TRACE_BUFFER_DLOG("AddPaddingRecord @ [%lu - %lu] %zu", wptr_ - begin(),
wptr_ - begin() + size, size);
- WriteChunkRecord(record, nullptr, size - sizeof(ChunkRecord));
+ WriteChunkRecord(wptr_, record, nullptr, size - sizeof(ChunkRecord));
// |wptr_| is deliberately not advanced when writing a padding record.
}
@@ -402,12 +484,20 @@
}
void TraceBuffer::SequenceIterator::MoveNext() {
+ // Stop iterating when we reach the end of the sequence.
// Note: |seq_begin| might be == |seq_end|.
if (cur == seq_end || cur->first.chunk_id == wrapping_id) {
cur = seq_end;
return;
}
+ // If the current chunk wasn't completed yet, we shouldn't advance past it as
+ // it may be rewritten with additional packets.
+ if (!cur->second.is_complete) {
+ cur = seq_end;
+ return;
+ }
+
ChunkID last_chunk_id = cur->first.chunk_id;
if (++cur == seq_end)
cur = seq_begin;
diff --git a/src/tracing/core/trace_buffer.h b/src/tracing/core/trace_buffer.h
index d4cce85..fba4306 100644
--- a/src/tracing/core/trace_buffer.h
+++ b/src/tracing/core/trace_buffer.h
@@ -157,18 +157,31 @@
~TraceBuffer();
// Copies a Chunk from a producer Shared Memory Buffer into the trace buffer.
- // |src| points to the first packet in the SharedMemoryABI's chunk shared
- // with an untrusted producer. "untrusted" here means: the producer might be
+ // |src| points to the first packet in the SharedMemoryABI's chunk shared with
+ // an untrusted producer. "untrusted" here means: the producer might be
// malicious and might change |src| concurrently while we read it (internally
- // this method memcpy()-s first the chunk before processing it).
- // None of the arguments should be trusted, unless otherwise stated. We can
- // trust that |src| points to a valid memory area, but not its contents.
+ // this method memcpy()-s first the chunk before processing it). None of the
+ // arguments should be trusted, unless otherwise stated. We can trust that
+ // |src| points to a valid memory area, but not its contents.
+ //
+ // This method may be called multiple times for the same chunk. In this case,
+ // the original chunk's payload will be overridden and its number of fragments
+ // and flags adjusted to match |num_fragments| and |chunk_flags|. The service
+ // may use this to insert partial chunks (|chunk_complete = false|) before the
+ // producer has committed them.
+ //
+ // If |chunk_complete| is |false|, the TraceBuffer will only consider the
+ // first |num_fragments - 1| packets to be complete, since the producer may
+ // not have finished writing the latest packet. Reading from a sequence will
+ // also not progress past any incomplete chunks until they were rewritten with
+ // |chunk_complete = true|, e.g. after a producer's commit.
void CopyChunkUntrusted(ProducerID producer_id_trusted,
uid_t producer_uid_trusted,
WriterID writer_id,
ChunkID chunk_id,
uint16_t num_fragments,
uint8_t chunk_flags,
+ bool chunk_complete,
const uint8_t* src,
size_t size);
// Applies a batch of |patches| to the given chunk, if the given chunk is
@@ -312,6 +325,8 @@
std::tie(other.producer_id, other.writer_id, other.chunk_id);
}
+ bool operator!=(const Key& other) const { return !(*this == other); }
+
// These fields should match at all times the corresponding fields in
// the |chunk_record|. They are copied here purely for efficiency to avoid
// dereferencing the buffer all the time.
@@ -320,18 +335,27 @@
ChunkID chunk_id;
};
- ChunkMeta(ChunkRecord* c, uint16_t p, uint8_t f, uid_t u)
- : chunk_record{c}, trusted_uid{u}, flags{f}, num_fragments{p} {}
+ ChunkMeta(ChunkRecord* r, uint16_t p, bool c, uint8_t f, uid_t u)
+ : chunk_record{r},
+ trusted_uid{u},
+ is_complete{c},
+ flags{f},
+ num_fragments{p} {}
ChunkRecord* const chunk_record; // Addr of ChunkRecord within |data_|.
const uid_t trusted_uid; // uid of the producer.
+ // If true, the chunk state was kChunkComplete at the time it was copied. If
+ // false, the chunk was still kChunkBeingWritten while copied. |is_complete|
+ // == false prevents the sequence to read past this chunk.
+ bool is_complete = false;
+
// Correspond to |chunk_record->flags| and |chunk_record->num_fragments|.
// Copied here for performance reasons (avoids having to dereference
// |chunk_record| while iterating over ChunkMeta) and to aid debugging in
// case the buffer gets corrupted.
uint8_t flags = 0; // See SharedMemoryABI::flags.
- const uint16_t num_fragments = 0; // Total number of packet fragments.
+ uint16_t num_fragments = 0; // Total number of packet fragments.
uint16_t num_fragments_read = 0; // Number of fragments already read.
@@ -476,7 +500,8 @@
// |src| can be nullptr (in which case |size| must be ==
// record.size - sizeof(ChunkRecord)), for the case of writing a padding
// record. |wptr_| is NOT advanced by this function, the caller must do that.
- void WriteChunkRecord(const ChunkRecord& record,
+ void WriteChunkRecord(uint8_t* wptr,
+ const ChunkRecord& record,
const uint8_t* src,
size_t size) {
// Note: |record.size| will be slightly bigger than |size| because of the
@@ -488,21 +513,21 @@
PERFETTO_DCHECK(record.size % sizeof(record) == 0);
PERFETTO_DCHECK(record.size >= size + sizeof(record));
PERFETTO_CHECK(record.size <= size_to_end());
- DcheckIsAlignedAndWithinBounds(wptr_);
+ DcheckIsAlignedAndWithinBounds(wptr);
// We may be writing to this area for the first time.
- data_.EnsureCommitted(static_cast<size_t>(wptr_ + record.size - begin()));
+ data_.EnsureCommitted(static_cast<size_t>(wptr + record.size - begin()));
// Deliberately not a *D*CHECK.
- PERFETTO_CHECK(wptr_ + sizeof(record) + size <= end());
- memcpy(wptr_, &record, sizeof(record));
+ PERFETTO_CHECK(wptr + sizeof(record) + size <= end());
+ memcpy(wptr, &record, sizeof(record));
if (PERFETTO_LIKELY(src)) {
- memcpy(wptr_ + sizeof(record), src, size);
+ memcpy(wptr + sizeof(record), src, size);
} else {
PERFETTO_DCHECK(size == record.size - sizeof(record));
}
const size_t rounding_size = record.size - sizeof(record) - size;
- memset(wptr_ + sizeof(record) + size, 0, rounding_size);
+ memset(wptr + sizeof(record) + size, 0, rounding_size);
}
uint8_t* begin() const { return reinterpret_cast<uint8_t*>(data_.Get()); }
diff --git a/src/tracing/core/trace_buffer_unittest.cc b/src/tracing/core/trace_buffer_unittest.cc
index 31cd6e5..dbd4ee0 100644
--- a/src/tracing/core/trace_buffer_unittest.cc
+++ b/src/tracing/core/trace_buffer_unittest.cc
@@ -185,16 +185,16 @@
TEST_F(TraceBufferTest, ReadWrite_FillTillEnd) {
ResetBuffer(4096);
for (int i = 0; i < 3; i++) {
- ASSERT_EQ(512u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ ASSERT_EQ(512u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(i * 4))
.AddPacket(512 - 16, 'a')
.CopyIntoTraceBuffer());
- ASSERT_EQ(512u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ ASSERT_EQ(512u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(i * 4 + 1))
.AddPacket(512 - 16, 'b')
.CopyIntoTraceBuffer());
- ASSERT_EQ(1024u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(2))
+ ASSERT_EQ(1024u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(i * 4 + 2))
.AddPacket(1024 - 16, 'c')
.CopyIntoTraceBuffer());
- ASSERT_EQ(2048u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(3))
+ ASSERT_EQ(2048u, CreateChunk(ProducerID(1), WriterID(1), ChunkID(i * 4 + 3))
.AddPacket(2048 - 16, 'd')
.CopyIntoTraceBuffer());
@@ -892,7 +892,7 @@
uint8_t valid_ptr = 0;
trace_buffer()->CopyChunkUntrusted(
ProducerID(1), uid_t(0), WriterID(1), ChunkID(1), 1 /* num packets */,
- 0 /* flags*/, &valid_ptr, sizeof(valid_ptr));
+ 0 /* flags */, true /* chunk_complete */, &valid_ptr, sizeof(valid_ptr));
CreateChunk(ProducerID(1), WriterID(1), ChunkID(2))
.AddPacket(32, 'b')
@@ -917,20 +917,6 @@
ASSERT_THAT(ReadPacket(), IsEmpty());
}
-TEST_F(TraceBufferTest, Malicious_RepeatedChunkID) {
- ResetBuffer(4096);
- SuppressSanityDchecksForTesting();
- CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
- .AddPacket(2048, 'a')
- .CopyIntoTraceBuffer();
- CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
- .AddPacket(1024, 'b')
- .CopyIntoTraceBuffer();
- trace_buffer()->BeginRead();
- ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(1024, 'b')));
- ASSERT_THAT(ReadPacket(), IsEmpty());
-}
-
TEST_F(TraceBufferTest, Malicious_DeclareMorePacketsBeyondBoundaries) {
ResetBuffer(4096);
SuppressSanityDchecksForTesting();
@@ -1009,9 +995,9 @@
std::vector<uint8_t> chunk;
chunk.insert(chunk.end(), 128 - sizeof(ChunkRecord), 0xff);
chunk.back() = 0x7f;
- trace_buffer()->CopyChunkUntrusted(ProducerID(4), uid_t(0), WriterID(1),
- ChunkID(1), 1 /* num packets */,
- 0 /* flags*/, chunk.data(), chunk.size());
+ trace_buffer()->CopyChunkUntrusted(
+ ProducerID(4), uid_t(0), WriterID(1), ChunkID(1), 1 /* num packets */,
+ 0 /* flags*/, true /* chunk_complete */, chunk.data(), chunk.size());
// Add a valid chunk.
CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
@@ -1036,7 +1022,7 @@
for (int i = 0; i < 3; i++) {
trace_buffer()->CopyChunkUntrusted(
ProducerID(1), uid_t(0), WriterID(1), ChunkID(1), 1 /* num packets */,
- 0 /* flags*/, chunk.data(), chunk.size());
+ 0 /* flags */, true /* chunk_complete */, chunk.data(), chunk.size());
}
trace_buffer()->BeginRead();
@@ -1122,6 +1108,96 @@
}
}
+TEST_F(TraceBufferTest, Malicious_OverrideWithShorterChunkSize) {
+ ResetBuffer(4096);
+ SuppressSanityDchecksForTesting();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(2048, 'a')
+ .CopyIntoTraceBuffer();
+ // The service should ignore this override of the chunk since the chunk size
+ // is different.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(1024, 'b')
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(2048, 'a')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Malicious_OverrideWithShorterChunkSizeAfterRead) {
+ ResetBuffer(4096);
+ SuppressSanityDchecksForTesting();
+
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(30, 'a')
+ .AddPacket(40, 'b')
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'b')));
+
+ // The service should ignore this override of the chunk since the chunk size
+ // is different.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(10, 'a')
+ .AddPacket(10, 'b')
+ .AddPacket(10, 'c')
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Test that the service didn't get stuck in some indeterminate state.
+ // Writing a valid chunk with a larger ID should make things work again.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(10, 'd')
+ .AddPacket(10, 'e')
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(10, 'd')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(10, 'e')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Malicious_OverrideWithDifferentOffsetAfterRead) {
+ ResetBuffer(4096);
+ SuppressSanityDchecksForTesting();
+
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(30, 'a')
+ .AddPacket(40, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'b')));
+
+ // The attacker in this case speculates on the fact that the read pointer is
+ // @ 70 which is >> the size of the new chunk we overwrite.
+ // The service will not discard this override since the chunk size is correct.
+ // However, it should detect that the packet headers at the current read
+ // offset are invalid and skip the read of this chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(10, 'a')
+ .AddPacket(10, 'b')
+ .AddPacket(10, 'c')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Test that the service didn't get stuck in some indeterminate state.
+ // Writing a valid chunk with a larger ID should make things work again.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(10, 'd')
+ .AddPacket(10, 'e')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(10, 'd')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(10, 'e')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
// -------------------
// SequenceIterator tests
// -------------------
@@ -1196,6 +1272,319 @@
ASSERT_TRUE(IteratorSeqEq(ProducerID(3), WriterID(1), {Neg(-1), 0, 1}));
}
+// -------------------
+// Re-writing same chunk id
+// -------------------
+
+TEST_F(TraceBufferTest, Override_ReCommitBeforeRead) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(100, 'a')
+ .AddPacket(100, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(100, 'a')
+ .AddPacket(100, 'b')
+ .AddPacket(100, 'c')
+ .AddPacket(100, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(100, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(100, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(100, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(100, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitAfterPartialRead) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitAfterFullRead) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+
+ // Overriding a complete packet here would trigger a DCHECK because the packet
+ // was already marked as complete.
+ SuppressSanityDchecksForTesting();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+// See also the Malicious_Override* tests above.
+TEST_F(TraceBufferTest, Override_ReCommitInvalid) {
+ ResetBuffer(4096);
+ SuppressSanityDchecksForTesting();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+
+ // This should not happen when the producer behaves correctly, since it
+ // shouldn't change the contents of chunk 0 after having allocated chunk 1.
+ //
+ // Since we've already started reading from chunk 1, TraceBuffer will
+ // recognize this and discard the override.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'e')
+ .AddPacket(60, 'f')
+ .AddPacket(70, 'g')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitReordered) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+
+ // Recommit chunk 0 and add chunk 1, but do this out of order.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(50, 'd')
+ .AddPacket(60, 'e')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .AddPacket(40, 'c')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(60, 'e')));
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitReorderedFragmenting) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+
+ // Recommit chunk 0 and add chunk 1, but do this out of order.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(50, 'd', kContFromPrevChunk)
+ .AddPacket(60, 'e')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .AddPacket(40, 'c', kContOnNextChunk)
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c'),
+ FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(60, 'e')));
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitSameBeforeRead) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ // Commit again the same chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ // Then write some new content in a new chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ // The reader should keep reading from the new chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitSameAfterRead) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+
+ // This re-commit should be ignored. We just re-committed an identical chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ // Then write some new content in a new chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+
+ // The reader should keep reading from the new chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitIncompleteAfterReadOutOfOrder) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ // The last packet in an incomplete chunk should be ignored as the producer
+ // may not have completed writing it.
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Then write some new content in a new chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(40, 'c')
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ // The read still shouldn't be advancing past the incomplete chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Recommit the original chunk with no changes but mark as complete.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b')
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/true);
+
+ // Reading should resume from the now completed chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
+TEST_F(TraceBufferTest, Override_ReCommitIncompleteFragmenting) {
+ ResetBuffer(4096);
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b', kContOnNextChunk)
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/false);
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(20, 'a')));
+ // The last packet in an incomplete chunk should be ignored as the producer
+ // may not have completed writing it.
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Then write some new content in a new chunk.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(1))
+ .AddPacket(40, 'c', kContFromPrevChunk)
+ .AddPacket(50, 'd')
+ .PadTo(512)
+ .CopyIntoTraceBuffer();
+ // The read still shouldn't be advancing past the incomplete chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+
+ // Recommit the original chunk with no changes but mark as complete.
+ CreateChunk(ProducerID(1), WriterID(1), ChunkID(0))
+ .AddPacket(20, 'a')
+ .AddPacket(30, 'b', kContOnNextChunk)
+ .PadTo(512)
+ .CopyIntoTraceBuffer(/*chunk_complete=*/true);
+
+ // Reading should resume from the now completed chunk.
+ trace_buffer()->BeginRead();
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(30, 'b'),
+ FakePacketFragment(40, 'c')));
+ ASSERT_THAT(ReadPacket(), ElementsAre(FakePacketFragment(50, 'd')));
+ ASSERT_THAT(ReadPacket(), IsEmpty());
+}
+
// TODO(primiano): test stats().
// TODO(primiano): test multiple streams interleaved.
// TODO(primiano): more testing on packet merging.
diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc
index 8912798..3b2af8f 100644
--- a/src/tracing/core/tracing_service_impl.cc
+++ b/src/tracing/core/tracing_service_impl.cc
@@ -1118,6 +1118,7 @@
BufferID buffer_id,
uint16_t num_fragments,
uint8_t chunk_flags,
+ bool chunk_complete,
const uint8_t* src,
size_t size) {
PERFETTO_DCHECK_THREAD(thread_checker_);
@@ -1163,7 +1164,8 @@
}
buf->CopyChunkUntrusted(producer_id_trusted, producer_uid_trusted, writer_id,
- chunk_id, num_fragments, chunk_flags, src, size);
+ chunk_id, num_fragments, chunk_flags, chunk_complete,
+ src, size);
}
void TracingServiceImpl::ApplyChunkPatches(
@@ -1579,7 +1581,7 @@
service_->CopyProducerPageIntoLogBuffer(
id_, uid_, writer_id, chunk_id, buffer_id, num_fragments, chunk_flags,
- chunk.payload_begin(), chunk.payload_size());
+ /*chunk_complete=*/true, chunk.payload_begin(), chunk.payload_size());
// This one has release-store semantics.
shmem_abi_.ReleaseChunkAsFree(std::move(chunk));
diff --git a/src/tracing/core/tracing_service_impl.h b/src/tracing/core/tracing_service_impl.h
index fba97fe..dc7f214 100644
--- a/src/tracing/core/tracing_service_impl.h
+++ b/src/tracing/core/tracing_service_impl.h
@@ -186,6 +186,7 @@
BufferID,
uint16_t num_fragments,
uint8_t chunk_flags,
+ bool chunk_complete,
const uint8_t* src,
size_t size);
void ApplyChunkPatches(ProducerID,