Fix GC dynamic ETLX replay

src/TraceEvent/Parsers/GCDynamicTraceEventParser.cs

@@ -3,6 +3,7 @@
 using System.Diagnostics;
 using System.Text;
 
+using Microsoft.Diagnostics.Tracing.Etlx;
 using Microsoft.Diagnostics.Tracing.Parsers.GCDynamic;
 
 namespace Microsoft.Diagnostics.Tracing.Parsers
@@ -128,10 +129,8 @@ internal GCDynamicTraceEventImpl(Action<GCDynamicTraceEventImpl> action, int eve
 
         /// <summary>
         /// These are the raw payload fields of the underlying event.  They read through
-        /// the cached <see cref="_payload"/> that <see cref="FixupData"/> populates per
-        /// event; when the payload didn't pass the bounds check (i.e. <c>_payload</c>
-        /// is <c>null</c>) the accessors return type-appropriate safe defaults rather
-        /// than throwing, so the dispatch hot path stays exception-safe.
+        /// the cached <see cref="_payload"/> populated before the typed event is accessed.
+        /// Invalid layouts return type-appropriate safe defaults.
         /// </summary>
         internal string Name { get { return _payload?.Name ?? string.Empty; } }
         internal Int32 DataSize { get { return _payload?.DataSize ?? 0; } }
@@ -145,12 +144,10 @@ internal GCDynamicTraceEventImpl(Action<GCDynamicTraceEventImpl> action, int eve
         /// The single <see cref="GCDynamicTraceEventImpl"/> instance is reused for every
         /// GCDynamic event the source produces, so the per-event payload validation
         /// must run here (not once at construction) -- it refreshes <see cref="_payload"/>
-        /// for the event that's about to dispatch.  Centralising the bounds check here
-        /// lets all downstream payload accessors (Name / DataSize / Data / ClrInstanceID,
-        /// the typed CommittedUsage fixed-offset reads, PayloadValue / PayloadValues,
-        /// ToXml) trust the cached layout without re-validating, and prevents an
-        /// attacker-controlled DataSize from triggering an OOB read or an unhandled
-        /// exception on the dispatch hot path.
+        /// for the raw event that's about to be converted and prevents an
+        /// attacker-controlled DataSize from selecting an incompatible template.
+        /// Payload accessors validate independently because TraceLog replay skips this
+        /// method after the converted event type has been persisted.
         /// </summary>
         internal override void FixupData()
         {
@@ -186,6 +183,13 @@ public GCDynamicEventBase EventPayload
         {
             get
             {
+                // TraceLog replay reuses this template and does not call FixupData, so
+                // refresh the layout before exposing the typed payload for this event.
+                if (traceEventSource is TraceLog)
+                {
+                    _payload = ReadPayloadLayout();
+                }
+
                 if (eventID == GCDynamicEventBase.CommittedUsageTemplate.ID)
                 {
                     return _committedUsageTemplate.Bind(this);
@@ -222,10 +226,8 @@ public override StringBuilder ToXml(StringBuilder sb)
 
         private event Action<GCDynamicTraceEventImpl> Action;
 
-        // Per-event scratch state populated by FixupData.  null means the bound event's
-        // payload failed the bounds check; accessors above return safe defaults in
-        // that case.  The cache lifetime is exactly one dispatch -- FixupData
-        // overwrites it before each event is delivered.
+        // Per-event scratch state refreshed by FixupData during raw conversion and by
+        // EventPayload during TraceLog replay.
         private PayloadLayout? _payload;
 
         /// <summary>
@@ -438,12 +440,12 @@ public sealed class CommittedUsageTraceEvent : GCDynamicEventBase
         /// </summary>
         internal const int MinimumDataSize = 42;
 
-        public short Version { get { return BitConverter.ToInt16(DataField, 0); } }
-        public long TotalCommittedInUse { get { return BitConverter.ToInt64(DataField, 2); } }
-        public long TotalCommittedInGlobalDecommit { get { return BitConverter.ToInt64(DataField, 10); } }
-        public long TotalCommittedInFree { get { return BitConverter.ToInt64(DataField, 18); } }
-        public long TotalCommittedInGlobalFree { get { return BitConverter.ToInt64(DataField, 26); } }
-        public long TotalBookkeepingCommitted { get { return BitConverter.ToInt64(DataField, 34); } }
+        public short Version { get { return GetInt16(0); } }
+        public long TotalCommittedInUse { get { return GetInt64(2); } }
+        public long TotalCommittedInGlobalDecommit { get { return GetInt64(10); } }
+        public long TotalCommittedInFree { get { return GetInt64(18); } }
+        public long TotalCommittedInGlobalFree { get { return GetInt64(26); } }
+        public long TotalBookkeepingCommitted { get { return GetInt64(34); } }
 
         internal override TraceEventID ID => TraceEventID.Illegal - 11;
         internal override string TaskName => "GC";
@@ -499,6 +501,18 @@ internal override IEnumerable<KeyValuePair<string, object>> PayloadValues
                 yield return new KeyValuePair<string, object>("TotalBookkeepingCommitted", TotalBookkeepingCommitted);
             }
         }
+
+        private short GetInt16(int offset)
+        {
+            byte[] data = DataField;
+            return data.Length >= offset + sizeof(short) ? BitConverter.ToInt16(data, offset) : (short)0;
+        }
+
+        private long GetInt64(int offset)
+        {
+            byte[] data = DataField;
+            return data.Length >= offset + sizeof(long) ? BitConverter.ToInt64(data, offset) : 0;
+        }
     }
 
     public sealed class CommittedUsage

src/TraceEvent/TraceEvent.Tests/Regression/GCDynamicTraceEventParserTests.cs

@@ -1,4 +1,5 @@
 using Microsoft.Diagnostics.Tracing;
+using Microsoft.Diagnostics.Tracing.Etlx;
 using Microsoft.Diagnostics.Tracing.Parsers;
 using Microsoft.Diagnostics.Tracing.Parsers.GCDynamic;
 using System;
@@ -143,6 +144,79 @@ public void GCDynamicCommittedUsageWithValidDataIsDispatched()
             });
         }
 
+        [Fact]
+        public void GCDynamicCommittedUsageReplayDecodesWithoutFixupData()
+        {
+            byte[] data = CreateCommittedUsageData(1, 100, 200, 300, 400, 500);
+            byte[] payload = CreatePayload("CommittedUsage", data.Length, data, 7);
+
+            WithGCDynamicReplayEvent(payload, delegate (GCDynamicTraceEventImpl traceEvent)
+            {
+                CommittedUsageTraceEvent committedUsage = (CommittedUsageTraceEvent)traceEvent.EventPayload;
+
+                Assert.Equal(1, committedUsage.Version);
+                Assert.Equal(100, committedUsage.TotalCommittedInUse);
+                Assert.Equal(500, committedUsage.TotalBookkeepingCommitted);
+            });
+        }
+
+        [Fact]
+        public void GCDynamicCommittedUsageReplayDoesNotReusePreviousPayload()
+        {
+            byte[] firstData = CreateCommittedUsageData(1, 100, 200, 300, 400, 500);
+            byte[] secondData = CreateCommittedUsageData(2, 600, 700, 800, 900, 1000);
+            byte[] firstPayload = CreatePayload("CommittedUsage", firstData.Length, firstData, 7);
+            byte[] secondPayload = CreatePayload("CommittedUsage", secondData.Length, secondData, 8);
+            List<short> versions = new List<short>();
+            List<long> committedInUse = new List<long>();
+
+            WithReusedGCDynamicReplayEvent(firstPayload, secondPayload, delegate (GCDynamicTraceEventImpl traceEvent)
+            {
+                CommittedUsageTraceEvent committedUsage = (CommittedUsageTraceEvent)traceEvent.EventPayload;
+                versions.Add(committedUsage.Version);
+                committedInUse.Add(committedUsage.TotalCommittedInUse);
+            });
+
+            Assert.Equal(new short[] { 1, 2 }, versions);
+            Assert.Equal(new long[] { 100, 600 }, committedInUse);
+        }
+
+        [Fact]
+        public void GCDynamicCommittedUsageReplayWithTruncatedDataIsSafe()
+        {
+            byte[] payload = CreatePayload("CommittedUsage", 1, new byte[] { 1 }, 7);
+
+            WithGCDynamicReplayEvent(payload, delegate (GCDynamicTraceEventImpl traceEvent)
+            {
+                CommittedUsageTraceEvent committedUsage = (CommittedUsageTraceEvent)traceEvent.EventPayload;
+                Assert.Equal(0, committedUsage.Version);
+                Assert.Equal(0, committedUsage.TotalCommittedInUse);
+                Assert.Equal(0, committedUsage.TotalCommittedInGlobalDecommit);
+                Assert.Equal(0, committedUsage.TotalCommittedInFree);
+                Assert.Equal(0, committedUsage.TotalCommittedInGlobalFree);
+                Assert.Equal(0, committedUsage.TotalBookkeepingCommitted);
+
+                List<KeyValuePair<string, object>> values = new List<KeyValuePair<string, object>>();
+                Exception payloadValuesException = Record.Exception(delegate
+                {
+                    foreach (KeyValuePair<string, object> value in committedUsage.PayloadValues)
+                    {
+                        values.Add(value);
+                    }
+                });
+                Assert.Null(payloadValuesException);
+                Assert.Equal(6, values.Count);
+                Assert.Equal((short)0, values[0].Value);
+                for (int i = 1; i < values.Count; i++)
+                {
+                    Assert.Equal((long)0, values[i].Value);
+                }
+
+                Exception toXmlException = Record.Exception(delegate { traceEvent.ToXml(new StringBuilder()); });
+                Assert.Null(toXmlException);
+            });
+        }
+
         /// <summary>
         /// Regression test for the propagated-exception path through
         /// PayloadValues on a malformed payload.  ToXml iterates
@@ -243,6 +317,18 @@ private static byte[] CreatePayload(string name, int dataSize, byte[] data, shor
             return payload.ToArray();
         }
 
+        private static byte[] CreateCommittedUsageData(short version, long committedInUse, long committedInGlobalDecommit, long committedInFree, long committedInGlobalFree, long bookkeepingCommitted)
+        {
+            byte[] data = new byte[CommittedUsageTraceEvent.MinimumDataSize];
+            BitConverter.GetBytes(version).CopyTo(data, 0);
+            BitConverter.GetBytes(committedInUse).CopyTo(data, 2);
+            BitConverter.GetBytes(committedInGlobalDecommit).CopyTo(data, 10);
+            BitConverter.GetBytes(committedInFree).CopyTo(data, 18);
+            BitConverter.GetBytes(committedInGlobalFree).CopyTo(data, 26);
+            BitConverter.GetBytes(bookkeepingCommitted).CopyTo(data, 34);
+            return data;
+        }
+
         private static unsafe void WithGCDynamicEvent(byte[] payload, Action<GCDynamicTraceEventImpl> action)
         {
             fixed (byte* payloadBytes = payload)
@@ -260,5 +346,44 @@ private static unsafe void WithGCDynamicEvent(byte[] payload, Action<GCDynamicTr
                 action(traceEvent);
             }
         }
+
+        private static unsafe void WithGCDynamicReplayEvent(byte[] payload, Action<GCDynamicTraceEventImpl> action)
+        {
+            WithReusedGCDynamicReplayEvent(payload, null, action);
+        }
+
+        private static unsafe void WithReusedGCDynamicReplayEvent(byte[] firstPayload, byte[] secondPayload, Action<GCDynamicTraceEventImpl> action)
+        {
+            fixed (byte* firstPayloadBytes = firstPayload)
+            fixed (byte* secondPayloadBytes = secondPayload)
+            {
+                TraceLog source = (TraceLog)Activator.CreateInstance(typeof(TraceLog), true);
+                source._QPCFreq = 1;
+                source._syncTimeQPC = 1;
+                source._syncTimeUTC = DateTime.UtcNow;
+                source.sessionStartTimeQPC = 1;
+
+                TraceEventNativeMethods.EVENT_RECORD eventRecord = new TraceEventNativeMethods.EVENT_RECORD();
+                eventRecord.EventHeader.ProviderId = GCDynamicTraceEventParser.ProviderGuid;
+                eventRecord.EventHeader.Id = (ushort)GCDynamicEventBase.CommittedUsageTemplate.ID;
+
+                GCDynamicTraceEventImpl traceEvent = new GCDynamicTraceEventImpl(null, (int)GCDynamicEventBase.CommittedUsageTemplate.ID, 1, "GC", Guid.Empty, 41, "CommittedUsage", GCDynamicTraceEventParser.ProviderGuid, "Microsoft-Windows-DotNETRuntime");
+                traceEvent.eventRecord = &eventRecord;
+                traceEvent.traceEventSource = source;
+
+                eventRecord.UserDataLength = (ushort)firstPayload.Length;
+                eventRecord.UserData = (IntPtr)firstPayloadBytes;
+                traceEvent.userData = eventRecord.UserData;
+                action(traceEvent);
+
+                if (secondPayload != null)
+                {
+                    eventRecord.UserDataLength = (ushort)secondPayload.Length;
+                    eventRecord.UserData = (IntPtr)secondPayloadBytes;
+                    traceEvent.userData = eventRecord.UserData;
+                    action(traceEvent);
+                }
+            }
+        }
     }
 }