Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

🤖 Copilot Code Review — PR #124428

Holistic Assessment

Motivation: Legitimate bug fix for a confirmed regression (#113061) found by fuzzing. When XmlReader.Create(stream) encounters non-ASCII bytes (0x80–0xFF) inside an XML declaration on a UTF-8 stream, UnDecodeChars() computes an inflated bytePos that exceeds bytesUsed, causing ArgumentOutOfRangeException in BlockCopy instead of the expected XmlException. The issue is real and the root cause analysis is correct.

Approach: The fix correctly identifies the mismatch between SafeAsciiDecoder's 1:1 byte-to-char mapping and Encoding.UTF8.GetByteCount()'s multi-byte count for chars > U+007F. It uses charPos directly for the byte offset (correct for SafeAsciiDecoder) and then validates that the target encoding agrees. This is the right layer and the right approach — minimal, targeted, and addresses root cause.

Summary: ⚠️ Needs Human Review. The core fix is correct and well-motivated. I have two observations worth human attention: (1) the bytePos mutation happens before the validation throw, which is safe because Throw is [DoesNotReturn] but is worth a human confirming is intentional; (2) a minor question about whether the issue's original repro (valid multi-byte UTF-8 0xC2 0xBF, not bare 0xBF) should also have a test case. Neither observation is blocking.

Detailed Findings

✅ Core Fix — Correct byte position computation

The central fix is sound:

• SafeAsciiDecoder always maps 1 byte → 1 char (confirmed in XmlEncoding.cs:175-183), so _ps.bytePos += _ps.charPos is the correct byte offset when this decoder is active.
• _ps.encoding is always UTF-8 when SafeAsciiDecoder is used (confirmed in SetupEncoding — _ps.encoding = Encoding.UTF8 paired with new SafeAsciiDecoder()), so the GetByteCount validation targets the right encoding.
• The check encodingByteCount != _ps.charPos correctly detects any non-ASCII byte: UTF-8 encodes U+0080–U+00FF as 2 bytes, so GetByteCount > charPos whenever a byte > 0x7F was decoded by SafeAsciiDecoder.
• False positives are impossible: if all bytes are 0x00–0x7F, UTF-8 encodes each in 1 byte, so encodingByteCount == charPos. Only truly invalid-for-UTF-8 inputs trigger the error.

✅ Async path coverage

The async code (XmlTextReaderImplAsync.cs:1004) calls the same UnDecodeChars() method. No separate async version exists. Both sync and async paths are covered by this single fix.

✅ Exception handling — Throw is [DoesNotReturn]

Throw(string res) at line 2674 is annotated [DoesNotReturn] and unconditionally throws XmlException. The fact that _ps.bytePos is mutated before the throw is harmless — the exception prevents any subsequent use of the corrupted state, and XmlReader does not support recovery after XmlException.

✅ Error message — Appropriate and consistent

SR.Xml_InvalidCharInThisEncoding ("Invalid character in the given encoding.") is the same error used throughout the codebase for encoding-related invalid input (e.g., lines 3661, 3879, and multiple test assertions in ReaderEncodingTests.cs). This is the correct exception type (XmlException) and message for this scenario.

✅ Test — Correctly verifies the fix

The test constructs a byte stream with 0xBF (an invalid standalone UTF-8 continuation byte) embedded in the version attribute value of an XML declaration. Key observations:

• Assert.Throws<XmlException> confirms the correct exception type (was ArgumentOutOfRangeException before the fix).
• Assert.Contains(_invalidCharInThisEncoding, ex.Message) confirms the correct error message.
• The while (reader.Read()) loop ensures the exception is caught regardless of which Read() call triggers it.
• The byte 0xBF cannot produce a passing test accidentally — it's the only non-ASCII byte and is what triggers the mismatch.

💡 Test coverage — Consider adding the original issue's repro scenario

The original issue's repro uses System.Text.Encoding.UTF8.GetBytes("<?xml version=\"1.0\xbf\"?>") which produces valid multi-byte UTF-8 0xC2 0xBF (not bare 0xBF). SafeAsciiDecoder would decode these as two separate chars (U+00C2, U+00BF), both > 0x7F, inflating GetByteCount by 2 extra bytes. This is a slightly different input than the test's single 0xBF. Consider adding an [InlineData]-style test (or [Theory]) covering both scenarios to match the original report.

💡 Test style — Regression comment citing issue number

The test comment references dotnet/runtime#113061. The repo conventions recommend avoiding regression comments citing GitHub issue numbers unless explicitly asked. This matches the existing pattern in the file (line 10 references an issue URL), so it's not inconsistent — but worth noting for the area maintainer's preference.