Fix GC dynamic ETLX replay (#2440)

PerfView 3.2.4 can throw `ArgumentOutOfRangeException` when GC Stats or
Heap Analyzer replays `GC/CommittedUsage` events from ETLX. During
conversion, `FixupData()` classifies the raw dynamic event and the ETLX
persists the synthetic event ID, but the original dynamic payload
envelope remains unchanged. On replay, the typed template therefore
still needs to parse that envelope.

This change refreshes the payload layout in `EventPayload` only when
processing a `TraceLog`. Raw ETW and EventPipe dispatch continue using
the payload prepared by `FixupData()`, avoiding redundant parsing.
Fixed-offset `CommittedUsage` fields also return safe defaults for
truncated payloads.

Regression coverage includes valid ETLX replay without `FixupData()`,
reused replay templates with distinct payloads, and malformed payload
access through properties, `PayloadValues`, and `ToXml`.

Fixes #2438

Author: brianrob

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);
+                }
+            }
+        }
     }
 }