From c38b71146b561c2996fbbcd4d7e0fef8686780db Mon Sep 17 00:00:00 2001
From: Johannes Schlatow <johannes.schlatow@genode-labs.com>
Date: Fri, 20 May 2022 12:00:09 +0200
Subject: [PATCH] trace_buffer: only iterate after initialization

There is a race between the trace subject doing the buffer
initialization and the monitor trying to iterate the buffer entries. If
the monitor tries to iterate entries of an uninitialized buffer, it will
read the very first entry twice. The monitor should therefore only start
iteration when the buffer has been initialised.

genodelabs/genode#4513
---
 repos/base/include/base/trace/buffer.h | 27 +++++++++++++++++++-------
 repos/os/include/trace/trace_buffer.h  | 13 ++++++++-----
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/repos/base/include/base/trace/buffer.h b/repos/base/include/base/trace/buffer.h
index 3ddbd999e8..59222f5e89 100644
--- a/repos/base/include/base/trace/buffer.h
+++ b/repos/base/include/base/trace/buffer.h
@@ -251,6 +251,9 @@ class Genode::Trace::Simple_buffer
 
 			public:
 
+				/* return invalid entry (checked by last()) */
+				static Entry invalid()     { return Entry(0); }
+
 				size_t      length() const { return _entry->len; }
 				char const *data()   const { return _entry->data; }
 
@@ -264,8 +267,18 @@ class Genode::Trace::Simple_buffer
 				bool head()     const { return !last() && _entry->head(); }
 		};
 
+		/* Return whether buffer has been initialized. */
+		bool initialized() const { return _size && _head_offset <= _size; }
+
 		/* Return the very first entry at the start of the buffer. */
-		Entry first() const { return Entry(_entries); }
+		Entry first() const
+		{
+			/* return invalid entry if buffer is uninitialised */
+			if (!initialized())
+				return Entry::invalid();
+
+			return Entry(_entries);
+		}
 
 		/**
 		 * Return the entry that follows the given entry.
@@ -278,14 +291,14 @@ class Genode::Trace::Simple_buffer
 		Entry next(Entry entry) const
 		{
 			if (entry.last() || entry._padding())
-				return Entry(0);
+				return Entry::invalid();
 
 			if (entry.head())
 				return entry;
 
 			addr_t const offset = (addr_t)entry.data() - (addr_t)_entries;
 			if (offset + entry.length() + sizeof(_Entry) > _size)
-				return Entry(0);
+				return Entry::invalid();
 
 			return Entry((_Entry const *)((addr_t)entry.data() + entry.length()));
 		}
@@ -382,11 +395,11 @@ class Genode::Trace::Partitioned_buffer
 		 ** Functions called from the TRACE client **
 		 ********************************************/
 
-		unsigned wrapped() { return _wrapped; }
+		unsigned           wrapped()      const { return _wrapped; }
+		unsigned long long lost_entries() const { return _lost_entries; }
 
-		unsigned long long lost_entries() { return _lost_entries; }
-
-		Entry first() const { return _consumer().first(); }
+		Entry first()       const { return _consumer().first(); }
+		bool  initialized() const { return _secondary_offset > 0 && _consumer().initialized(); }
 
 		/**
 		 * Return the entry that follows the given entry.
diff --git a/repos/os/include/trace/trace_buffer.h b/repos/os/include/trace/trace_buffer.h
index 13c39e61d6..a12e4d015a 100644
--- a/repos/os/include/trace/trace_buffer.h
+++ b/repos/os/include/trace/trace_buffer.h
@@ -24,9 +24,10 @@
 class Trace_buffer
 {
 	private:
+		using Entry = Genode::Trace::Buffer::Entry;
 
 		Genode::Trace::Buffer        &_buffer;
-		Genode::Trace::Buffer::Entry  _curr { _buffer.first() };
+		Entry                         _curr { Entry::invalid() };
 		unsigned long long            _lost_count { 0 };
 
 	public:
@@ -41,6 +42,9 @@ class Trace_buffer
 		{
 			using namespace Genode;
 
+			if (!_buffer.initialized())
+				return;
+
 			bool lost = _buffer.lost_entries() != _lost_count;
 			if (lost) {
 				warning("lost ", _buffer.lost_entries() - _lost_count,
@@ -48,18 +52,17 @@ class Trace_buffer
 				_lost_count = (unsigned)_buffer.lost_entries();
 			}
 
-			Trace::Buffer::Entry entry { _curr };
+			Entry entry { _curr };
 
 			/**
 			 * Iterate over all entries that were not processed yet.
 			 *
 			 * A note on terminology: The head of the buffer marks the write
 			 * position. The first entry is the one that starts at the lowest
-			 * memory address. The next entry returns an invalid entry called
-			 * if the 'last' end of the buffer (highest address) was reached.
+			 * memory address. The next entry returns an invalid entry
+			 * if the 'last' entry of the buffer (highest address) was reached.
 			 */
 			for (; !entry.head(); entry = _buffer.next(entry)) {
-
 				/* continue at first entry if we hit the end of the buffer */
 				if (entry.last())
 					entry = _buffer.first();