diff --git a/src/TraceEvent/Parsers/GCDynamicTraceEventParser.cs b/src/TraceEvent/Parsers/GCDynamicTraceEventParser.cs index 81fd0f2bd..3791ddf86 100644 --- a/src/TraceEvent/Parsers/GCDynamicTraceEventParser.cs +++ b/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 action, int eve /// /// These are the raw payload fields of the underlying event. They read through - /// the cached that populates per - /// event; when the payload didn't pass the bounds check (i.e. _payload - /// is null) the accessors return type-appropriate safe defaults rather - /// than throwing, so the dispatch hot path stays exception-safe. + /// the cached populated before the typed event is accessed. + /// Invalid layouts return type-appropriate safe defaults. /// internal string Name { get { return _payload?.Name ?? string.Empty; } } internal Int32 DataSize { get { return _payload?.DataSize ?? 0; } } @@ -145,12 +144,10 @@ internal GCDynamicTraceEventImpl(Action action, int eve /// The single 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 - /// 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. /// 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 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; /// @@ -438,12 +440,12 @@ public sealed class CommittedUsageTraceEvent : GCDynamicEventBase /// 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> PayloadValues yield return new KeyValuePair("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 diff --git a/src/TraceEvent/TraceEvent.Tests/Regression/GCDynamicTraceEventParserTests.cs b/src/TraceEvent/TraceEvent.Tests/Regression/GCDynamicTraceEventParserTests.cs index a0cbaea0a..28fb8cbed 100644 --- a/src/TraceEvent/TraceEvent.Tests/Regression/GCDynamicTraceEventParserTests.cs +++ b/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 versions = new List(); + List committedInUse = new List(); + + 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> values = new List>(); + Exception payloadValuesException = Record.Exception(delegate + { + foreach (KeyValuePair 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); + }); + } + /// /// 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 action) { fixed (byte* payloadBytes = payload) @@ -260,5 +346,44 @@ private static unsafe void WithGCDynamicEvent(byte[] payload, Action action) + { + WithReusedGCDynamicReplayEvent(payload, null, action); + } + + private static unsafe void WithReusedGCDynamicReplayEvent(byte[] firstPayload, byte[] secondPayload, Action 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); + } + } + } } }