Skip to content

Fix TRX logger mangling astral-plane (non-BMP) characters #16318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Azat Mukhametshin (azat-msft) merged 2 commits into from
Jul 31, 2026
Merged

Fix TRX logger mangling astral-plane (non-BMP) characters #16318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9419d7
Fix TRX logger mangling astral-plane (non-BMP) characters
azat-msft Jul 28, 2026
c19d82a
Use manual surrogate-aware scan instead of regex
azat-msft Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions src/Microsoft.TestPlatform.Extensions.TrxLogger/XML/XmlPersistence.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using System.Linq;
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;
using System.Xml;

using Microsoft.TestPlatform.Extensions.TrxLogger.ObjectModel;
Expand Down Expand Up @@ -675,20 +674,48 @@ private static void SaveSimpleData(object? value, XmlNode nodeToSaveAt, object?

// From xml spec (http://www.w3.org/TR/xml/#charsets) valid chars:
// #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF]
//
// In .NET a string is a sequence of UTF-16 code units, so characters in the
// [#x10000-#x10FFFF] (astral) range are represented by a surrogate pair: a high
// surrogate (\uD800-\uDBFF) followed by a low surrogate (\uDC00-\uDFFF). Such a
// pair is perfectly valid XML and must be preserved as-is, so we look ahead with
// char.IsSurrogatePair and copy both code units through untouched.
//
// A *lone* surrogate, on the other hand, is not a valid Unicode scalar value and
// is not valid XML - XmlWriter would throw on it - so it still has to be escaped.
// That is why we cannot simply treat \uD800-\uDFFF as valid: doing so would trade
// a mangled-string bug for a "we emit unparseable XML" bug, which is worse because
// it breaks every consumer of the trx at read time. Only properly paired
// surrogates are allowed through.
StringBuilder? builder = null;
for (int i = 0; i < str.Length; i++)
{
// IsSurrogatePair also covers the end-of-string case, so a high surrogate as
// the last character correctly falls through to the invalid branch below.
if (char.IsSurrogatePair(str, i))
{
builder?.Append(str[i]).Append(str[i + 1]);

@Evangelink Amaury Levé (Evangelink) Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: builder?.Append(str, i, 2) says the same thing in one call and makes it obvious the two code units are copied as a unit. The chained form is also slightly subtle in that the null-conditional short-circuits the whole chain — correct here, but it's the kind of thing that looks like a bug on a fast read.

i++;
continue;
}

// we are handling only #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD]
// because C# support unicode character in range \u0000 to \uFFFF
MatchEvaluator evaluator = new(ReplaceInvalidCharacterWithUniCodeEscapeSequence);
string invalidChar = @"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD]";
return Regex.Replace(str, invalidChar, evaluator);
}
char c = str[i];
if (IsValidXmlChar(c))
{
builder?.Append(c);
continue;
}

private static string ReplaceInvalidCharacterWithUniCodeEscapeSequence(Match match)
{
char x = match.Value[0];
return $@"\u{(ushort)x:x4}";
builder ??= new StringBuilder(str.Length).Append(str, 0, i);

@Evangelink Amaury Levé (Evangelink) Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this capacity hint is guaranteed to be too small. We only ever get here because a char needs escaping, and each escaped code unit expands 1 -> 6 chars, so the builder will always have to grow at least once.

testfx solved the identical problem in TrxReportEngine.Utilities.cs with length + length / 2 plus an overflow guard:

private static int GetInvalidXmlCharBuilderCapacity(int length)
{
    int extraCapacity = length / 2;
    return length <= int.MaxValue - extraCapacity ? length + extraCapacity : length;
}

They also use the new StringBuilder(str, 0, i, capacity) constructor, which does the prefix copy as part of construction instead of a separate Append. Worth matching, both for the allocation and because it keeps the two implementations diffable.

builder.Append(@"\u").Append(((ushort)c).ToString("x4", CultureInfo.InvariantCulture));
}

return builder?.ToString() ?? str;
}

private static bool IsValidXmlChar(char c)
=> c is '\x09' or '\x0A' or '\x0D' or (>= '\x20' and <= '\uD7FF') or (>= '\uE000' and <= '\uFFFD');

private XmlNode? EnsureLocationExists(XmlElement xml, string location, string? nameSpaceUri)
{
XmlNode? node = LocationToXmlNode(xml, location);
Expand Down
131 changes: 124 additions & 7 deletions test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/XmlPersistenceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Xml;

using Microsoft.TestPlatform.Extensions.TrxLogger.XML;
using Microsoft.VisualStudio.TestTools.UnitTesting;

Expand All @@ -15,23 +17,138 @@ public void SaveObjectShouldReplaceInvalidCharacter()
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

// we are handling only #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD]
char[] invalidXmlCharacterArray = new char[7];
// we are handling only #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] plus
// well-formed surrogate pairs, which encode [#x10000-#x10FFFF].
char[] invalidXmlCharacterArray = new char[5];
invalidXmlCharacterArray[0] = (char)0x5;
invalidXmlCharacterArray[1] = (char)0xb;
invalidXmlCharacterArray[2] = (char)0xf;
invalidXmlCharacterArray[3] = (char)0xd800;
invalidXmlCharacterArray[4] = (char)0xdc00;
invalidXmlCharacterArray[5] = (char)0xfffe;
invalidXmlCharacterArray[6] = (char)0x0;
invalidXmlCharacterArray[3] = (char)0xfffe;
invalidXmlCharacterArray[4] = (char)0x0;

string strWithInvalidCharForXml = new(invalidXmlCharacterArray);
XmlPersistence.SaveObject(strWithInvalidCharForXml, node, null, "dummy");

string expectedResult = "\\u0005\\u000b\\u000f\\ud800\\udc00\\ufffe\\u0000";
string expectedResult = "\\u0005\\u000b\\u000f\\ufffe\\u0000";
Assert.AreEqual(0, string.Compare(expectedResult, node.InnerXml));
}

[TestMethod]
public void SaveObjectShouldNotReplaceAdjacentHighAndLowSurrogate()
{
// 0xd800 immediately followed by 0xdc00 is not two invalid characters - it is the
// UTF-16 encoding of U+10000, which the XML spec lists as valid. It must round-trip
// unescaped. This case used to be asserted the other way around.
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

string validSurrogatePair = new(new[] { (char)0xd800, (char)0xdc00 });
XmlPersistence.SaveObject(validSurrogatePair, node, null, "dummy");

Assert.AreEqual(validSurrogatePair, node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldNotReplaceWellFormedSurrogatePairInElementText()
{
// A surrogate pair encodes a character in [#x10000-#x10FFFF], which the XML spec
// lists as valid, so it must survive untouched. Here U+1F389 (🎉).
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject("party \U0001F389 done", node, null, "dummy");

Assert.AreEqual("party \U0001F389 done", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldNotReplaceWellFormedSurrogatePairInAttributeValue()
{
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

xmlPersistence.SaveObject("party \U0001F389 done", node, "@testName", null);

Assert.AreEqual("party \U0001F389 done", node.GetAttribute("testName"));
}

[TestMethod]
public void SaveObjectShouldReplaceLoneHighSurrogate()
{
// Unlike a pair, a lone high surrogate is not a valid Unicode scalar value and is
// not valid XML (XmlWriter would throw on it), so it must still be escaped.
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject("a" + (char)0xd800 + "b", node, null, "dummy");

Assert.AreEqual("a\\ud800b", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldReplaceLoneLowSurrogate()
{
// A low surrogate that is not preceded by a high surrogate is equally invalid.
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject((char)0xdc00 + "b", node, null, "dummy");

Assert.AreEqual("\\udc00b", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldReplaceHighSurrogateAtEndOfString()

@Evangelink Amaury Levé (Evangelink) Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broader point about the test strategy rather than this specific test: every new test targets XmlPersistence in isolation, but the premise of the PR is about the bytes that end up in the .trx file. Nothing here would catch a regression outside the sanitizer — e.g. someone setting CheckCharacters = false on the XmlWriterSettings in TrxLogger.PopulateTrxFile, or a future change that writes a string into the DOM without going through SaveSimpleData.

TrxLoggerTests already has a PopulateTrxFile override harness that writes a real file. One test through that path — emoji in a test name, write, re-parse, assert the name survived — would pin the thing users actually reported, and it's cheap. Not a blocker for this PR, but I'd like it before this pattern gets copied again.

{
// The pair-matching logic must not read past the end of the string.
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject("ab" + (char)0xd800, node, null, "dummy");

Assert.AreEqual("ab\\ud800", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldHandleMixOfValidPairLoneSurrogateAndInvalidCharacters()

@Evangelink Amaury Levé (Evangelink) Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mixed-input test is the best one in the set — it's the one that would actually catch a regression in the backfill index.

Three edges are still unpinned, and they're precisely the ones that break if someone later "simplifies" the surrogate branch:

  • two consecutive high surrogates, (char)0xd800 + (char)0xd801 — both must be escaped
  • the inverted order, (char)0xdc00 + (char)0xd800 — low then high, neither forms a pair
  • the empty string, which must return without allocating

All three behave correctly today (I checked), so this is purely about locking the behaviour down. A single [DataRow]-driven test would cover them and let you collapse some of the near-duplicate cases above.

{
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject("\U0001F389" + (char)0xd800 + "\v" + (char)0xdc00, node, null, "dummy");

// The pair is preserved; the lone high surrogate, the vertical tab and the
// trailing lone low surrogate are all escaped.
Assert.AreEqual("\U0001F389\\ud800\\u000b\\udc00", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldNotReplaceNonAsciiBmpCharacters()
{
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

XmlPersistence.SaveObject("Grüße 日本語 Čau", node, null, "dummy");

Assert.AreEqual("Grüße 日本語 Čau", node.InnerXml);
}

[TestMethod]
public void SaveObjectShouldProduceWellFormedXmlWithAstralCharacters()
{
XmlPersistence xmlPersistence = new();
var node = xmlPersistence.CreateRootElement("TestRun");

xmlPersistence.SaveObject("party \U0001F389 done", node, "@testName", null);
XmlPersistence.SaveObject("output \U0001F389 here", node, null, "dummy");

XmlDocument document = new();
document.LoadXml(node.OuterXml);

Assert.AreEqual("party \U0001F389 done", document.DocumentElement!.GetAttribute("testName"));
Assert.AreEqual("output \U0001F389 here", document.DocumentElement.InnerText);
}

[TestMethod]
public void SaveObjectShouldNotReplaceValidCharacter()
{
Expand Down