From f66467f882ef6592ad5bd739158e116b74650438 Mon Sep 17 00:00:00 2001 From: "Marius Sutara (from Dev Box)" Date: Tue, 15 Sep 2026 13:54:48 -0700 Subject: [PATCH 1/3] Harden dynamic payload length parsing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/TraceEvent/DynamicTraceEventParser.cs | 68 ++- ...raceEventParserVariableLengthFieldTests.cs | 517 ++++++++++++++++++ 2 files changed, 566 insertions(+), 19 deletions(-) create mode 100644 src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs diff --git a/src/TraceEvent/DynamicTraceEventParser.cs b/src/TraceEvent/DynamicTraceEventParser.cs index 3163f9934..e42a74bd8 100644 --- a/src/TraceEvent/DynamicTraceEventParser.cs +++ b/src/TraceEvent/DynamicTraceEventParser.cs @@ -616,6 +616,7 @@ private object GetPayloadValueAt(ref PayloadFetch payloadFetch, int offset, int case TypeCode.String: { var isAnsi = false; + long stringSize = size; if (size >= SPECIAL_SIZES) { isAnsi = ((size & IS_ANSI) != 0); @@ -636,7 +637,7 @@ private object GetPayloadValueAt(ref PayloadFetch payloadFetch, int offset, int // The length-prefix bytes are read from the payload via GetInt16At / GetInt32At, // which do not bounds check. Validate that the prefix itself is inside the payload // before reading it, so a crafted offset near EventDataLength cannot pull bytes - // from adjacent native memory into 'size'. + // from adjacent native memory into 'stringSize'. int prefixBytes = ((size & BIT_32) != 0) ? 4 : 2; if (offset < 0 || EventDataLength - offset < prefixBytes) { @@ -644,17 +645,17 @@ private object GetPayloadValueAt(ref PayloadFetch payloadFetch, int offset, int } if (((size & BIT_32) != 0)) { - size = (ushort)GetInt32At(offset); + stringSize = (uint)GetInt32At(offset); offset += 4; // skip size; } else { - size = (ushort)GetInt16At(offset); + stringSize = (ushort)GetInt16At(offset); offset += 2; // skip size; } if (unicodeByteCountString) { - size /= 2; // Unicode string with BYTE count. Element count is half that. + stringSize /= 2; // Unicode string with BYTE count. Element count is half that. } } else @@ -664,26 +665,26 @@ private object GetPayloadValueAt(ref PayloadFetch payloadFetch, int offset, int } else if (size > 0x8000) // What is this? looks like a hack. { - size -= 0x8000; + stringSize -= 0x8000; isAnsi = true; } - // Bounds-check before reading the fixed/counted string. For counted strings 'size' was just + // Bounds-check before reading the fixed/counted string. For counted strings 'stringSize' was just // read from the payload bytes, so we must ensure the read stays inside EventDataLength to - // avoid an out-of-bounds read of native heap memory. ANSI strings use 'size' bytes; Unicode - // strings use 'size * 2' bytes. We compute the required byte count using long arithmetic to + // avoid an out-of-bounds read of native heap memory. ANSI strings use 'stringSize' bytes; Unicode + // strings use 'stringSize * 2' bytes. We compute the required byte count using long arithmetic to // avoid any chance of overflow before the comparison. - long bytesNeeded = isAnsi ? (long)size : (long)size * 2; + long bytesNeeded = isAnsi ? stringSize : stringSize * 2; if (offset < 0 || EventDataLength - offset < bytesNeeded) { - throw new ArgumentOutOfRangeException(nameof(size)); + throw new ArgumentOutOfRangeException(nameof(stringSize)); } if (isAnsi) { - return GetFixedAnsiStringAt(size, offset); + return GetFixedAnsiStringAt((int)stringSize, offset); } else { - return GetFixedUnicodeStringAt(size, offset); + return GetFixedUnicodeStringAt((int)stringSize, offset); } } case TypeCode.Boolean: @@ -1183,6 +1184,12 @@ private int SkipToField(PayloadFetch[] payloadFetches, int targetFieldIdx, int s return payloadLength; } + // Reject cached or nested negative offsets before an unchecked field scan. + if (fieldOffset < 0) + { + throw new ArgumentOutOfRangeException(nameof(fieldOffset)); + } + // This can be N*N but because of our cache, it is not in the common case when you fetch // fields in order. while (fieldIdx < targetFieldIdx) @@ -1237,6 +1244,7 @@ private int GetCountForArray(PayloadFetch payloadFetch, PayloadFetchArrayInfo ar { throw new ArgumentOutOfRangeException(nameof(arrayCount)); } + ValidateArrayCount(arrayInfo, offset, arrayCount); } else if (arrayInfo.Kind == ArrayKind.LengthPrefixed) { @@ -1254,17 +1262,26 @@ private int GetCountForArray(PayloadFetch payloadFetch, PayloadFetchArrayInfo ar { throw new ArgumentOutOfRangeException(nameof(offset)); } + // Length prefixes are unsigned, including values with their high bit set. + long count; if (((payloadFetch.Size & DynamicTraceEventData.BIT_32) != 0)) { - arrayCount = GetInt32At(offset); + count = (uint)GetInt32At(offset); offset += 4; } else { - arrayCount = GetInt16At(offset); + count = (ushort)GetInt16At(offset); offset += 2; } + // Bound the unsigned count before narrowing it to int. + if (count > EventDataLength) + { + throw new ArgumentOutOfRangeException(nameof(count)); + } + arrayCount = (int)count; + // The length field is read directly from the payload bytes, // so validate it against EventDataLength before the caller uses it to read array elements. // For fixed-size elements we can compute the exact byte cost; for variable-sized elements @@ -1330,7 +1347,7 @@ private int GetCountForArray(PayloadFetch payloadFetch, PayloadFetchArrayInfo ar return (dataOffset, arrayByteLength / elementSize); } - // Validates that 'arrayCount' (just read from payload bytes for a length-prefixed + // Validates that 'arrayCount' (from a fixed-count or length-prefixed // array) does not point past the end of the event payload. For fixed-size elements we use the actual // element size; for variable-sized elements we use a per-element minimum derived from the element // encoding (e.g. 2 bytes for UTF-16 null-terminated strings) so that callers that fast-path byte / @@ -1440,15 +1457,23 @@ internal int OffsetOfNextField(ref PayloadFetch payloadFetch, int offset, int pa } else if (IsCountedSize(size) && payloadFetch.Type == typeof(string)) { - int elemSize; + // The length prefix is read with unchecked raw reads, so validate it is in bounds first. + int prefixBytes = ((size & BIT_32) != 0) ? 4 : 2; + if (offset < 0 || payloadLength - offset < prefixBytes) + { + throw new ArgumentOutOfRangeException(nameof(offset)); + } + + // Preserve unsigned prefixes and widen before scaling UTF-16 element counts. + long elemSize; if (((size & BIT_32) != 0)) { - elemSize = GetInt32At(offset); + elemSize = (uint)GetInt32At(offset); offset += 4; // skip size; } else { - elemSize = GetInt16At(offset); + elemSize = (ushort)GetInt16At(offset); offset += 2; // skip size; } if ((size & IS_ANSI) == 0 && (size & ELEM_COUNT) != 0) @@ -1456,7 +1481,12 @@ internal int OffsetOfNextField(ref PayloadFetch payloadFetch, int offset, int pa elemSize *= 2; // Counted (not byte counted) unicode string. chars are 2 wide. } - return offset + elemSize; + if (elemSize > payloadLength - offset) + { + throw new ArgumentOutOfRangeException(nameof(elemSize)); + } + + return offset + (int)elemSize; } else if (size == VARINT) { diff --git a/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs new file mode 100644 index 000000000..95ab75840 --- /dev/null +++ b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs @@ -0,0 +1,517 @@ +using Microsoft.Diagnostics.Tracing; +using Microsoft.Diagnostics.Tracing.Parsers; + +using System; +using System.Collections.Generic; +using System.Reflection; +using System.Text; + +using Xunit; + +namespace TraceEventTests +{ + /// + /// Covers variable-length field traversal, lookup bounds, and offset caching. + /// + public class DynamicTraceEventParserVariableLengthFieldTests + { + private const ushort CountedUnicodeByteCount = DynamicTraceEventData.COUNTED_SIZE; + private const ushort CountedUnicodeElemCount = DynamicTraceEventData.COUNTED_SIZE | DynamicTraceEventData.ELEM_COUNT; + private const ushort LengthPrefixedArray = DynamicTraceEventData.COUNTED_SIZE | DynamicTraceEventData.ELEM_COUNT; + private const BindingFlags PrivateInstance = BindingFlags.Instance | BindingFlags.NonPublic; + private const string LookupError = "<<>>"; + + public static IEnumerable CountedStringCases() + { + foreach (ushort size in CountedStringEncodings()) + { + foreach (uint count in new uint[] { 0, 2, 16384, 32765, 32766, 32767, 32768, 33932, 65532 }) + { + int prefixBytes = (size & DynamicTraceEventData.BIT_32) != 0 ? 4 : 2; + int byteCount = checked((int)count * BytesPerCount(size)); + if (prefixBytes + byteCount <= ushort.MaxValue) + { + yield return new object[] { size, count }; + } + } + } + } + + public static IEnumerable InvalidCountedStringCases() + { + foreach (ushort size in CountedStringEncodings()) + { + yield return new object[] { size, 64u }; + if ((size & DynamicTraceEventData.BIT_32) != 0) + { + foreach (uint count in new uint[] { 0x10000, 0x10001, 0x7FFFFFFF, 0x80000000, 0x80000001, uint.MaxValue }) + { + yield return new object[] { size, count }; + } + } + } + + yield return new object[] { CountedUnicodeElemCount, 0x8000u }; + } + + [Theory] + [MemberData(nameof(CountedStringCases))] + public void CountedString_TraversalAndLookupAgree(ushort size, uint count) + { + bool widePrefix = (size & DynamicTraceEventData.BIT_32) != 0; + bool isAnsi = (size & DynamicTraceEventData.IS_ANSI) != 0; + int prefixBytes = widePrefix ? 4 : 2; + int byteCount = checked((int)count * BytesPerCount(size)); + string expected = new string('x', isAnsi ? byteCount : byteCount / 2); + byte[] payload = new byte[prefixBytes + byteCount]; + WriteCount(payload, 0, count, widePrefix); + byte[] text = (isAnsi ? Encoding.ASCII : Encoding.Unicode).GetBytes(expected); + Buffer.BlockCopy(text, 0, payload, prefixBytes, text.Length); + var fetch = StringFetch(0, size); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Equal(payload.Length, traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); + Assert.Equal(expected, ReadValue(traceEvent, fetch, 0)); + Assert.Equal(expected, traceEvent.PayloadValue(0)); + }); + } + + [Theory] + [MemberData(nameof(InvalidCountedStringCases))] + public void CountedString_LengthExceedsPayload_BothPathsReject(ushort size, uint count) + { + bool widePrefix = (size & DynamicTraceEventData.BIT_32) != 0; + byte[] payload = new byte[(widePrefix ? 4 : 2) + 4]; + WriteCount(payload, 0, count, widePrefix); + var fetch = StringFetch(0, size); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); + Assert.IsType(error.InnerException); + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + }); + } + + [Theory] + [InlineData(false, 0)] + [InlineData(false, 1)] + [InlineData(false, 32767)] + [InlineData(false, 32768)] + [InlineData(false, 65000)] + [InlineData(true, 0)] + [InlineData(true, 1)] + [InlineData(true, 32767)] + [InlineData(true, 32768)] + [InlineData(true, 65000)] + public void LengthPrefixedArray_TraversalAndLookupAgree(bool widePrefix, int elementCount) + { + int prefixBytes = widePrefix ? 4 : 2; + byte[] payload = new byte[prefixBytes + elementCount]; + WriteCount(payload, 0, (uint)elementCount, widePrefix); + if (elementCount > 0) + { + payload[payload.Length - 1] = 0x7A; + } + var fetch = ByteArrayFetch(widePrefix); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Equal(payload.Length, traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); + byte[] value = Assert.IsType(traceEvent.PayloadValue(0)); + Assert.Equal(elementCount, value.Length); + if (elementCount > 0) + { + Assert.Equal((byte)0x7A, value[elementCount - 1]); + } + }); + } + + [Theory] + [InlineData(false, 64u)] + [InlineData(true, 64u)] + [InlineData(true, 0x10000u)] + [InlineData(true, 0x7FFFFFFFu)] + [InlineData(true, 0x80000000u)] + [InlineData(true, 0xFFFFFFFFu)] + public void LengthPrefixedArray_CountExceedsPayload_BothPathsReject(bool widePrefix, uint count) + { + byte[] payload = new byte[(widePrefix ? 4 : 2) + 4]; + WriteCount(payload, 0, count, widePrefix); + var fetch = ByteArrayFetch(widePrefix); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + var traversalError = Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); + var lookupError = Assert.IsType(error.InnerException); + if (widePrefix && count > int.MaxValue) + { + Assert.Equal("count", traversalError.ParamName); + Assert.Equal("count", lookupError.ParamName); + } + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + }); + } + + [Theory] + [InlineData(false, 0, 0)] + [InlineData(false, 1, 0)] + [InlineData(false, 1, 3)] + [InlineData(true, 0, 0)] + [InlineData(true, 1, 0)] + [InlineData(true, 2, 0)] + [InlineData(true, 3, 0)] + [InlineData(true, 3, 3)] + public void LengthPrefix_Truncated_BothPathsReject(bool widePrefix, int remaining, int offset) + { + int length = offset + remaining; + byte[] payload = new byte[Math.Max(1, length)]; + ushort size = (ushort)(CountedUnicodeByteCount | (widePrefix ? DynamicTraceEventData.BIT_32 : 0)); + var fetches = new[] { StringFetch(0, size), ByteArrayFetch(widePrefix) }; + + WithEvent(payload, fetches, traceEvent => + { + foreach (var field in fetches) + { + var fetch = field; + Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, offset, length)); + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, offset)); + Assert.IsType(error.InnerException); + } + }, length); + } + + [Theory] + [InlineData(-1)] + [InlineData(-31470)] + public void OffsetOfNextField_NegativeOffset_Throws(int offset) + { + byte[] payload = new byte[64]; + var fetch = StringFetch(0, CountedUnicodeByteCount); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, offset, payload.Length)); + }); + } + + [Fact] + public void OffsetOfNextField_StructAtNegativeOffset_Throws() + { + // The nested null-terminated field otherwise scans from the invalid starting offset. + byte[] payload = new byte[64]; + var fetch = StructFetch(StringFetch(0, DynamicTraceEventData.NULL_TERMINATED)); + + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, -31470, payload.Length)); + }); + } + + [Fact] + public void PayloadValue_NegativeCachedOffset_ReturnsError() + { + byte[] payload = Encoding.Unicode.GetBytes("a\0b\0"); + WithEvent(payload, StringFetches(2, DynamicTraceEventData.NULL_TERMINATED), traceEvent => + { + typeof(DynamicTraceEventData).GetField("cachedEventId", PrivateInstance).SetValue(traceEvent, traceEvent.EventIndex); + typeof(DynamicTraceEventData).GetField("cachedFieldIdx", PrivateInstance).SetValue(traceEvent, 0); + typeof(DynamicTraceEventData).GetField("cachedFieldOffset", PrivateInstance).SetValue(traceEvent, -31470); + Assert.Equal(LookupError, traceEvent.PayloadValue(1)); + }); + } + + [Theory] + [InlineData(false, "")] + [InlineData(false, "abc")] + [InlineData(true, "abc")] + public void FixedString_LookupPreservesEncoding(bool isAnsi, string expected) + { + ushort size = (ushort)(expected.Length + (isAnsi ? 0x8000 : 0)); + byte[] text = (isAnsi ? Encoding.ASCII : Encoding.Unicode).GetBytes(expected); + byte[] payload = text.Length == 0 ? new byte[1] : text; + var fetch = StringFetch(0, size); + WithEvent(payload, new[] { fetch }, traceEvent => + { + Assert.Equal(expected, ReadValue(traceEvent, fetch, 0)); + }); + } + + [Theory] + [InlineData(typeof(byte), 1)] + [InlineData(typeof(char), 1)] + [InlineData(typeof(char), 2)] + [InlineData(typeof(int), 4)] + public void FixedCountArray_ExactlyFillsPayload_Decodes(Type elementType, ushort elementSize) + { + const ushort offset = 3; + var element = new DynamicTraceEventData.PayloadFetch(0, elementSize, elementType); + var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(offset, element, 3); + byte[] payload = new byte[offset + 3 * elementSize]; + for (int i = 0; i < 3; i++) + { + payload[offset + i * elementSize] = (byte)('a' + i); + } + + WithEvent(payload, new[] { fetch }, traceEvent => + { + object value = ReadValue(traceEvent, fetch, offset); + if (elementType == typeof(char)) + { + Assert.Equal("abc", value); + } + else if (elementType == typeof(byte)) + { + Assert.Equal(new byte[] { 97, 98, 99 }, Assert.IsType(value)); + } + else + { + Assert.Equal(new[] { 97, 98, 99 }, Assert.IsType(value)); + } + Assert.Equal(value, traceEvent.PayloadValue(0)); + }); + } + + [Theory] + [InlineData(typeof(byte), 1, 0)] + [InlineData(typeof(byte), 1, 3)] + [InlineData(typeof(char), 1, 0)] + [InlineData(typeof(char), 1, 3)] + [InlineData(typeof(char), 2, 0)] + [InlineData(typeof(char), 2, 3)] + [InlineData(typeof(int), 4, 3)] + public void FixedCountArray_Truncated_ReturnsError(Type elementType, ushort elementSize, ushort offset) + { + var element = new DynamicTraceEventData.PayloadFetch(0, elementSize, elementType); + var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(offset, element, 3); + byte[] payload = new byte[offset + 3 * elementSize]; + + WithEvent(payload, new[] { fetch }, traceEvent => + { + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, offset)); + Assert.IsType(error.InnerException); + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + }, payload.Length - 1); + } + + [Fact] + public void FixedCountArray_TruncatedNullTerminatedString_RejectsBeforeScanning() + { + var element = StringFetch(0, DynamicTraceEventData.NULL_TERMINATED); + var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(0, element, 2); + // Two empty Unicode strings still need four bytes for their terminators. + byte[] payload = new byte[3]; + WithEvent(payload, new[] { fetch }, traceEvent => + { + var traversalError = Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); + Assert.Equal("arrayCount", traversalError.ParamName); + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); + Assert.Equal("arrayCount", Assert.IsType(error.InnerException).ParamName); + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + }); + } + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void FixedCountArray_VariableElements_ChecksMinimumSize(bool widePrefix, bool truncated) + { + ushort size = (ushort)(CountedUnicodeByteCount | (widePrefix ? DynamicTraceEventData.BIT_32 : 0)); + var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(0, StringFetch(0, size), 2); + byte[] payload = new byte[2 * (widePrefix ? 4 : 2)]; + int length = payload.Length - (truncated ? 1 : 0); + WithEvent(payload, new[] { fetch }, traceEvent => + { + if (truncated) + { + Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, length)); + var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); + Assert.IsType(error.InnerException); + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + } + else + { + Assert.Equal(length, traceEvent.OffsetOfNextField(ref fetch, 0, length)); + Assert.Equal(new[] { "", "" }, Assert.IsType(ReadValue(traceEvent, fetch, 0))); + Assert.Equal(new[] { "", "" }, Assert.IsType(traceEvent.PayloadValue(0))); + } + }, length); + } + + [Fact] + public void PayloadValue_EventWithOversizedCountedString_DecodesAllFollowingFields() + { + string first = new string('a', 65); + string big = new string('b', 16966); + string last = new string('d', 159); + byte[] payload = BuildCountedUnicodeStringPayload(first, big, "c", last); + Assert.Equal(34390, payload.Length); + var fetches = StringFetches(4, CountedUnicodeByteCount); + + WithEvent(payload, fetches, traceEvent => + { + object[] values = { traceEvent.PayloadValue(0), traceEvent.PayloadValue(1), traceEvent.PayloadValue(2), traceEvent.PayloadValue(3) }; + Assert.Equal(new object[] { first, big, "c", last }, values); + Assert.Equal(34066, traceEvent.OffsetOfNextField(ref fetches[1], 132, payload.Length)); + }); + } + + [Fact] + public void PayloadValue_EventWithOversizedCountedString_IsStableAcrossRepeatedWalks() + { + string first = new string('a', 65); + string big = new string('b', 16966); + byte[] payload = BuildCountedUnicodeStringPayload(first, big, "c", "d"); + + WithEvent(payload, StringFetches(4, CountedUnicodeByteCount), traceEvent => + { + for (int pass = 0; pass < 2; pass++) + { + object[] values = { traceEvent.PayloadValue(0), traceEvent.PayloadValue(1), traceEvent.PayloadValue(2), traceEvent.PayloadValue(3) }; + Assert.Equal(new object[] { first, big, "c", "d" }, values); + Assert.Equal("d", traceEvent.PayloadValue(3)); + } + }); + } + + [Fact] + public void PayloadValue_MalformedCountedString_FollowingLookupsRemainErrors() + { + byte[] payload = new byte[8]; + WriteCount(payload, 0, 64, false); + WithEvent(payload, StringFetches(3, CountedUnicodeByteCount), traceEvent => + { + for (int pass = 0; pass < 2; pass++) + { + Assert.Equal(LookupError, traceEvent.PayloadValue(1)); + Assert.Equal(LookupError, traceEvent.PayloadValue(2)); + } + }); + } + + private static IEnumerable CountedStringEncodings() + { + foreach (int flags in new[] { 0, 1, 2, 3, 8, 9, 10, 11 }) + { + yield return (ushort)(CountedUnicodeByteCount | flags); + } + } + + private static int BytesPerCount(ushort size) + { + return (size & DynamicTraceEventData.IS_ANSI) == 0 && (size & DynamicTraceEventData.ELEM_COUNT) != 0 ? 2 : 1; + } + + private static object ReadValue(DynamicTraceEventData traceEvent, DynamicTraceEventData.PayloadFetch fetch, int offset) + { + // Bypass the Debug-only pre-walk so it cannot mask a lookup-path regression. + return typeof(DynamicTraceEventData).GetMethod("GetPayloadValueAt", PrivateInstance) + .Invoke(traceEvent, new object[] { fetch, offset, traceEvent.EventDataLength }); + } + + private static DynamicTraceEventData.PayloadFetch StringFetch(ushort offset, ushort size) + { + return new DynamicTraceEventData.PayloadFetch(offset, size, typeof(string)); + } + + private static DynamicTraceEventData.PayloadFetch ByteArrayFetch(bool widePrefix) + { + var element = new DynamicTraceEventData.PayloadFetch(0, 1, typeof(byte)); + ushort size = (ushort)(LengthPrefixedArray | (widePrefix ? DynamicTraceEventData.BIT_32 : 0)); + return DynamicTraceEventData.PayloadFetch.ArrayPayloadFetch(0, element, size); + } + + private static DynamicTraceEventData.PayloadFetch StructFetch(DynamicTraceEventData.PayloadFetch field) + { + var classInfo = new DynamicTraceEventData.PayloadFetchClassInfo() + { + FieldFetches = new[] { field }, + FieldNames = new[] { "Inner" } + }; + + return DynamicTraceEventData.PayloadFetch.StructPayloadFetch(0, classInfo); + } + + private static DynamicTraceEventData.PayloadFetch[] StringFetches(int count, ushort size) + { + var fetches = new DynamicTraceEventData.PayloadFetch[count]; + fetches[0] = StringFetch(0, size); + for (int i = 1; i < count; i++) + { + fetches[i] = StringFetch(ushort.MaxValue, size); + } + + return fetches; + } + + private static byte[] BuildCountedUnicodeStringPayload(params string[] values) + { + int total = 0; + foreach (string value in values) + { + total += 2 + (value.Length * 2); + } + + byte[] payload = new byte[total]; + int offset = 0; + foreach (string value in values) + { + byte[] bytes = Encoding.Unicode.GetBytes(value); + WriteCount(payload, offset, checked((ushort)bytes.Length), false); + offset += 2; + Buffer.BlockCopy(bytes, 0, payload, offset, bytes.Length); + offset += bytes.Length; + } + + return payload; + } + + private static void WriteCount(byte[] buffer, int offset, uint value, bool widePrefix) + { + int prefixBytes = widePrefix ? 4 : 2; + Assert.True(widePrefix || value <= ushort.MaxValue); + for (int i = 0; i < prefixBytes; i++) + { + buffer[offset + i] = (byte)(value >> (8 * i)); + } + } + + private static unsafe void WithEvent(byte[] payload, DynamicTraceEventData.PayloadFetch[] fetches, Action action, int? eventDataLength = null) + { + int length = eventDataLength ?? payload.Length; + Assert.InRange(payload.Length, 1, ushort.MaxValue); + Assert.InRange(length, 0, payload.Length); + // Padding keeps the tested regression reads inside allocated storage. + const int padding = 32768; + byte[] storage = new byte[padding + payload.Length + padding]; + Buffer.BlockCopy(payload, 0, storage, padding, payload.Length); + + fixed (byte* storageBytes = storage) + { + TraceEventNativeMethods.EVENT_RECORD eventRecord = new TraceEventNativeMethods.EVENT_RECORD(); + eventRecord.EventHeader.Id = 1; + eventRecord.UserDataLength = checked((ushort)length); + eventRecord.UserData = (IntPtr)(storageBytes + padding); + + var names = new string[fetches.Length]; + for (int i = 0; i < fetches.Length; i++) + { + names[i] = "Field" + i; + } + + var traceEvent = new DynamicTraceEventData(null, 1, 0, "Task", Guid.Empty, 0, "Opcode", Guid.Empty, "Provider"); + traceEvent.payloadFetches = fetches; + traceEvent.payloadNames = names; + traceEvent.eventRecord = &eventRecord; + traceEvent.userData = eventRecord.UserData; + + action(traceEvent); + } + } + } +} From f1eb5ed1d862fd2383526db601f82854d7fa2307 Mon Sep 17 00:00:00 2001 From: "Marius Sutara (from Dev Box)" Date: Wed, 16 Sep 2026 09:40:58 -0700 Subject: [PATCH 2/3] Remove reflection from parser tests Preserve direct lookup coverage through public payload access and remove the synthetic private cache-state test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...raceEventParserVariableLengthFieldTests.cs | 96 ++++++++----------- 1 file changed, 38 insertions(+), 58 deletions(-) diff --git a/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs index 95ab75840..921349d02 100644 --- a/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs +++ b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Reflection; using System.Text; using Xunit; @@ -18,7 +17,6 @@ public class DynamicTraceEventParserVariableLengthFieldTests private const ushort CountedUnicodeByteCount = DynamicTraceEventData.COUNTED_SIZE; private const ushort CountedUnicodeElemCount = DynamicTraceEventData.COUNTED_SIZE | DynamicTraceEventData.ELEM_COUNT; private const ushort LengthPrefixedArray = DynamicTraceEventData.COUNTED_SIZE | DynamicTraceEventData.ELEM_COUNT; - private const BindingFlags PrivateInstance = BindingFlags.Instance | BindingFlags.NonPublic; private const string LookupError = "<<>>"; public static IEnumerable CountedStringCases() @@ -69,28 +67,25 @@ public void CountedString_TraversalAndLookupAgree(ushort size, uint count) Buffer.BlockCopy(text, 0, payload, prefixBytes, text.Length); var fetch = StringFetch(0, size); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { Assert.Equal(payload.Length, traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); - Assert.Equal(expected, ReadValue(traceEvent, fetch, 0)); Assert.Equal(expected, traceEvent.PayloadValue(0)); }); } [Theory] [MemberData(nameof(InvalidCountedStringCases))] - public void CountedString_LengthExceedsPayload_BothPathsReject(ushort size, uint count) + public void CountedString_LengthExceedsPayload_TraversalAndPayloadValueReject(ushort size, uint count) { bool widePrefix = (size & DynamicTraceEventData.BIT_32) != 0; byte[] payload = new byte[(widePrefix ? 4 : 2) + 4]; WriteCount(payload, 0, count, widePrefix); var fetch = StringFetch(0, size); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); - Assert.IsType(error.InnerException); Assert.Equal(LookupError, traceEvent.PayloadValue(0)); }); } @@ -117,7 +112,7 @@ public void LengthPrefixedArray_TraversalAndLookupAgree(bool widePrefix, int ele } var fetch = ByteArrayFetch(widePrefix); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { Assert.Equal(payload.Length, traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); byte[] value = Assert.IsType(traceEvent.PayloadValue(0)); @@ -136,21 +131,18 @@ public void LengthPrefixedArray_TraversalAndLookupAgree(bool widePrefix, int ele [InlineData(true, 0x7FFFFFFFu)] [InlineData(true, 0x80000000u)] [InlineData(true, 0xFFFFFFFFu)] - public void LengthPrefixedArray_CountExceedsPayload_BothPathsReject(bool widePrefix, uint count) + public void LengthPrefixedArray_CountExceedsPayload_TraversalAndPayloadValueReject(bool widePrefix, uint count) { byte[] payload = new byte[(widePrefix ? 4 : 2) + 4]; WriteCount(payload, 0, count, widePrefix); var fetch = ByteArrayFetch(widePrefix); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { var traversalError = Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); - var lookupError = Assert.IsType(error.InnerException); if (widePrefix && count > int.MaxValue) { Assert.Equal("count", traversalError.ParamName); - Assert.Equal("count", lookupError.ParamName); } Assert.Equal(LookupError, traceEvent.PayloadValue(0)); }); @@ -165,23 +157,30 @@ public void LengthPrefixedArray_CountExceedsPayload_BothPathsReject(bool widePre [InlineData(true, 2, 0)] [InlineData(true, 3, 0)] [InlineData(true, 3, 3)] - public void LengthPrefix_Truncated_BothPathsReject(bool widePrefix, int remaining, int offset) + public void LengthPrefix_Truncated_RejectsWhenReadable(bool widePrefix, int remaining, int offset) { int length = offset + remaining; byte[] payload = new byte[Math.Max(1, length)]; ushort size = (ushort)(CountedUnicodeByteCount | (widePrefix ? DynamicTraceEventData.BIT_32 : 0)); - var fetches = new[] { StringFetch(0, size), ByteArrayFetch(widePrefix) }; + var fetches = new[] { StringFetch((ushort)offset, size), ByteArrayFetch(widePrefix, (ushort)offset) }; - WithEvent(payload, fetches, traceEvent => + foreach (var field in fetches) { - foreach (var field in fetches) + var fetch = field; + WithEvent(payload, new[] { fetch }, traceEvent => { - var fetch = field; Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, offset, length)); - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, offset)); - Assert.IsType(error.InnerException); + }, length); + + if (length > offset) + { + var lookupFetch = field; + WithEvent(payload, LookupFetches(lookupFetch, length), traceEvent => + { + Assert.Equal(LookupError, traceEvent.PayloadValue(0)); + }, length); } - }, length); + } } [Theory] @@ -211,19 +210,6 @@ public void OffsetOfNextField_StructAtNegativeOffset_Throws() }); } - [Fact] - public void PayloadValue_NegativeCachedOffset_ReturnsError() - { - byte[] payload = Encoding.Unicode.GetBytes("a\0b\0"); - WithEvent(payload, StringFetches(2, DynamicTraceEventData.NULL_TERMINATED), traceEvent => - { - typeof(DynamicTraceEventData).GetField("cachedEventId", PrivateInstance).SetValue(traceEvent, traceEvent.EventIndex); - typeof(DynamicTraceEventData).GetField("cachedFieldIdx", PrivateInstance).SetValue(traceEvent, 0); - typeof(DynamicTraceEventData).GetField("cachedFieldOffset", PrivateInstance).SetValue(traceEvent, -31470); - Assert.Equal(LookupError, traceEvent.PayloadValue(1)); - }); - } - [Theory] [InlineData(false, "")] [InlineData(false, "abc")] @@ -234,9 +220,9 @@ public void FixedString_LookupPreservesEncoding(bool isAnsi, string expected) byte[] text = (isAnsi ? Encoding.ASCII : Encoding.Unicode).GetBytes(expected); byte[] payload = text.Length == 0 ? new byte[1] : text; var fetch = StringFetch(0, size); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { - Assert.Equal(expected, ReadValue(traceEvent, fetch, 0)); + Assert.Equal(expected, traceEvent.PayloadValue(0)); }); } @@ -256,9 +242,9 @@ public void FixedCountArray_ExactlyFillsPayload_Decodes(Type elementType, ushort payload[offset + i * elementSize] = (byte)('a' + i); } - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { - object value = ReadValue(traceEvent, fetch, offset); + object value = traceEvent.PayloadValue(0); if (elementType == typeof(char)) { Assert.Equal("abc", value); @@ -289,10 +275,8 @@ public void FixedCountArray_Truncated_ReturnsError(Type elementType, ushort elem var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(offset, element, 3); byte[] payload = new byte[offset + 3 * elementSize]; - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, offset)); - Assert.IsType(error.InnerException); Assert.Equal(LookupError, traceEvent.PayloadValue(0)); }, payload.Length - 1); } @@ -304,12 +288,10 @@ public void FixedCountArray_TruncatedNullTerminatedString_RejectsBeforeScanning( var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(0, element, 2); // Two empty Unicode strings still need four bytes for their terminators. byte[] payload = new byte[3]; - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, payload.Length), traceEvent => { var traversalError = Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, payload.Length)); Assert.Equal("arrayCount", traversalError.ParamName); - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); - Assert.Equal("arrayCount", Assert.IsType(error.InnerException).ParamName); Assert.Equal(LookupError, traceEvent.PayloadValue(0)); }); } @@ -325,19 +307,16 @@ public void FixedCountArray_VariableElements_ChecksMinimumSize(bool widePrefix, var fetch = DynamicTraceEventData.PayloadFetch.FixedCountArrayPayloadFetch(0, StringFetch(0, size), 2); byte[] payload = new byte[2 * (widePrefix ? 4 : 2)]; int length = payload.Length - (truncated ? 1 : 0); - WithEvent(payload, new[] { fetch }, traceEvent => + WithEvent(payload, LookupFetches(fetch, length), traceEvent => { if (truncated) { Assert.Throws(() => traceEvent.OffsetOfNextField(ref fetch, 0, length)); - var error = Assert.Throws(() => ReadValue(traceEvent, fetch, 0)); - Assert.IsType(error.InnerException); Assert.Equal(LookupError, traceEvent.PayloadValue(0)); } else { Assert.Equal(length, traceEvent.OffsetOfNextField(ref fetch, 0, length)); - Assert.Equal(new[] { "", "" }, Assert.IsType(ReadValue(traceEvent, fetch, 0))); Assert.Equal(new[] { "", "" }, Assert.IsType(traceEvent.PayloadValue(0))); } }, length); @@ -407,23 +386,24 @@ private static int BytesPerCount(ushort size) return (size & DynamicTraceEventData.IS_ANSI) == 0 && (size & DynamicTraceEventData.ELEM_COUNT) != 0 ? 2 : 1; } - private static object ReadValue(DynamicTraceEventData traceEvent, DynamicTraceEventData.PayloadFetch fetch, int offset) - { - // Bypass the Debug-only pre-walk so it cannot mask a lookup-path regression. - return typeof(DynamicTraceEventData).GetMethod("GetPayloadValueAt", PrivateInstance) - .Invoke(traceEvent, new object[] { fetch, offset, traceEvent.EventDataLength }); - } - private static DynamicTraceEventData.PayloadFetch StringFetch(ushort offset, ushort size) { return new DynamicTraceEventData.PayloadFetch(offset, size, typeof(string)); } - private static DynamicTraceEventData.PayloadFetch ByteArrayFetch(bool widePrefix) + private static DynamicTraceEventData.PayloadFetch ByteArrayFetch(bool widePrefix, ushort offset = 0) { var element = new DynamicTraceEventData.PayloadFetch(0, 1, typeof(byte)); ushort size = (ushort)(LengthPrefixedArray | (widePrefix ? DynamicTraceEventData.BIT_32 : 0)); - return DynamicTraceEventData.PayloadFetch.ArrayPayloadFetch(0, element, size); + return DynamicTraceEventData.PayloadFetch.ArrayPayloadFetch(offset, element, size); + } + + private static DynamicTraceEventData.PayloadFetch[] LookupFetches(DynamicTraceEventData.PayloadFetch target, int payloadLength) + { + Assert.InRange(payloadLength, 1, ushort.MaxValue); + // A later fixed offset lets Debug validation skip traversal of the target before its lookup is tested. + var sentinel = new DynamicTraceEventData.PayloadFetch((ushort)(payloadLength - 1), 1, typeof(byte)); + return new[] { target, sentinel }; } private static DynamicTraceEventData.PayloadFetch StructFetch(DynamicTraceEventData.PayloadFetch field) From d7c79b99463535e8ca4f7b706e48e6f38bcfe872 Mon Sep 17 00:00:00 2001 From: "Marius Sutara (from Dev Box)" Date: Thu, 17 Sep 2026 16:27:05 -0700 Subject: [PATCH 3/3] Clarify dynamic parser test sentinel Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs index 921349d02..b2e1c735d 100644 --- a/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs +++ b/src/TraceEvent/TraceEvent.Tests/Parsing/DynamicTraceEventParserVariableLengthFieldTests.cs @@ -401,7 +401,8 @@ private static DynamicTraceEventData.PayloadFetch ByteArrayFetch(bool widePrefix private static DynamicTraceEventData.PayloadFetch[] LookupFetches(DynamicTraceEventData.PayloadFetch target, int payloadLength) { Assert.InRange(payloadLength, 1, ushort.MaxValue); - // A later fixed offset lets Debug validation skip traversal of the target before its lookup is tested. + // Append a fixed-offset sentinel so PayloadValue's Debug-only full-payload validation does not + // traverse the target; this ensures the test exercises the target through GetPayloadValueAt. var sentinel = new DynamicTraceEventData.PayloadFetch((ushort)(payloadLength - 1), 1, typeof(byte)); return new[] { target, sentinel }; }