Skip to content

Commit 41b8fbe

Browse files
committed
fix(xml,xml-linq): reject code points outside XML's Char production (#2349)
Split from #2085, which repaired the NUL truncation and left this policy open. Measured: 28 of the 29 non-Char bytes in 0x00-0x1F were EMITTED RAW at every writer content door and every Xml.Linq direct door, so the emitted document was not well-formed XML. THE TICKET RECORDED FIVE PRICED OPTIONS AND CALLED IT A USER DECISION. The blocker was that /rv was absent. The reference collapses the table to one: if (_checkCharacters) throw XmlConvert.CreateInvalidCharException((char)ch, '\0'); else { ... entitize or write raw ... } -- XmlEncodedRawTextWriter.cs:1635-1653 with CheckCharacters defaulting to true (XmlWriterSettings.cs:513). .NET rejects by default and the flag is what turns that off. That is option B, DERIVED rather than chosen, and SA-5 covers it. BOTH OF THE TICKET'S PRICING COMPLICATIONS ARE DISSOLVED, NOT ACCEPTED. 1. It priced enforcement on XmlConvert::VerifyXmlChars, which iterates `char` and so checks BYTES -- it accepts U+FFFE, U+FFFF and a lone surrogate encoded in UTF-8, all genuinely outside Char, so "enforce with the validator we already ship" would have bought C0 controls only. Ticket #2354, EARLIER THE SAME DAY, moved a code-point decoder into Core.Base, and both modules/xml and modules/xml-linq already depend on it. The correct check now costs one call and no new component edge; the graph stays at 41 modules / 93 edges. 2. It priced option B as "not a same-shaped change on both sides", because XNode::SerializeTo takes no settings and SaveOptions has no such value. .NET'S DO NOT EITHER: XNode.GetXmlWriterSettings constructs a DEFAULT XmlWriterSettings and touches only Indent and NamespaceHandling (XNode.cs:681-687), inheriting CheckCharacters = true. So the Linq side needs no settings channel, no new SaveOptions value and no ambient default -- it checks unconditionally, and the two door families agree BY CONSTRUCTION rather than by coordination. That was the whole reason this looked like two changes. TWO EXCEPTION TYPES, DELIBERATELY. NUL keeps XmlException, because that is this port's own truncation guard (#2085) -- a length boundary at the tinyxml2 const char* API, not a transcription of a .NET check. Every other non-Char code point raises ArgumentException, because that is what XmlConvert.CreateInvalidCharException produces (XmlConvert.cs:1614-1622), carrying .NET's text. WHAT IS STILL OPEN IS STATED RATHER THAN LEFT IMPLICIT. XmlReaderSettings ::CheckCharacters also defaults true and is also unenforced, and this port's reader ACCEPTS these characters -- so writing and reading now disagree. Options A, B and D all leave that asymmetry and only C and E avoid it. .NET has none, because its reader enforces too; the reader here is tinyxml2, which this port does not drive character by character, so closing it is not a matter of adding a call. Both scope pins are UPDATED, not deleted, exactly as the ticket anticipated: NonNulControlCharacters_StillEmitted_PinnedScopeBoundary and NonNulControlCharacters_StillEmittedByTheDirectDoor become Fix2349_* on the same inputs. Two cases replacing two, so the count does not move. Four mutations, all caught, each at BOTH door families: the check goes byte-wise again; U+FFFE and U+FFFF become Char; tab/LF/CR stop being Char; the C0 controls become Char. Downstream: neither cna nor mobile-eggbert references XmlWriter, XDocument or XElement -- zero sites in both. Gate: 17,333 run, 17,333 passed, 0 failed, 0 skipped across 38 executables, GREEN. Module boundaries valid (41 modules, 93 edges). docs/Migration-XmlCheckCharacters.md
1 parent b43e72a commit 41b8fbe

8 files changed

Lines changed: 301 additions & 37 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — the writer doors reject characters outside XML's `Char` production (ticket #2349)
5+
6+
*2026-08-18.* `XmlWriter::WriteString("a\x01b")` and `XText("a\x01b").ToString()` now raise
7+
`ArgumentException`. They used to emit the byte raw, producing a document that is not well-formed
8+
XML.
9+
10+
Landed under `docs/StandingApprovals.md` SA-5. No signature, layout or `noexcept` change.
11+
12+
---
13+
14+
## 1. What changed
15+
16+
| Value | Was | Is |
17+
|---|---|---|
18+
| `0x01`, `0x08`, `0x0B`, `0x0C`, `0x0E``0x1F` in content | emitted raw | `ArgumentException` |
19+
| `U+FFFE`, `U+FFFF` | emitted raw | `ArgumentException` |
20+
| a lone surrogate in UTF-8 (`ED A0 80`) | emitted raw | `ArgumentException` |
21+
| `\t`, `\n`, `\r` | emitted | **unchanged** — they *are* `Char` |
22+
| any other text, including multi-byte || **unchanged, byte for byte** |
23+
| `NUL` | `XmlException` (#2085) | **unchanged** — §4 |
24+
25+
Measured before: **28 of the 29 non-`Char` bytes in `0x00``0x1F`** went through both door
26+
families.
27+
28+
## 2. The ticket recorded five priced options and called it a user decision
29+
30+
`docs/SystemXmlNamespaceReviewPlan.md` SS22 carries the table: *A reject unconditionally, B honour
31+
the flag, C keep today's permissiveness, D writer only, E widen the reader too.* The blocker was
32+
that `/rv` was absent.
33+
34+
**The reference collapses the table to one option.**
35+
`XmlWriterSettings.CheckCharacters` defaults to `true` (`XmlWriterSettings.cs:513`) and is
36+
enforced:
37+
38+
```csharp
39+
if (_checkCharacters) throw XmlConvert.CreateInvalidCharException((char)ch, '\0');
40+
else { … entitize or write raw … } // XmlEncodedRawTextWriter.cs:1635-1653
41+
```
42+
43+
.NET rejects by default; the flag is what turns that off. That is option B, derived rather than
44+
chosen.
45+
46+
## 3. Both of the ticket's pricing complications are dissolved, not accepted
47+
48+
**(1) "Enforcing with the validator we already ship buys C0 controls only."** True of
49+
`XmlConvert::VerifyXmlChars`, which iterates `char` and so checks **bytes** — it accepts `U+FFFE`,
50+
`U+FFFF` and a lone surrogate. Ticket **#2354**, earlier the same day, moved a **code-point**
51+
decoder into `Core.Base`, and both `modules/xml` and `modules/xml-linq` already depend on it. A
52+
code-point-correct check now costs one call and **no new component edge**; the graph stays at
53+
41/93. The ticket priced this when that decoder was not there.
54+
55+
**(2) "Option B is not a same-shaped change on both sides, because the Xml.Linq direct doors
56+
cannot see the flag."** **.NET's cannot either.** `XNode.GetXmlWriterSettings` constructs a
57+
*default* `XmlWriterSettings` and touches only `Indent` and `NamespaceHandling`
58+
(`XNode.cs:681-687`), so it inherits `CheckCharacters = true` and checks exactly as any
59+
default-settings writer does. The Linq side needs **no** new settings channel, no new
60+
`SaveOptions` value and no ambient default: it checks unconditionally, and the two door families
61+
agree **by construction** rather than by coordination.
62+
63+
## 4. Two exception types, deliberately
64+
65+
`NUL` keeps `XmlException`, because that is this port's own truncation guard (#2085) — a length
66+
boundary at the tinyxml2 `const char*` API, not a transcription of a .NET check. Every other
67+
non-`Char` code point raises `ArgumentException`, because that is what
68+
`XmlConvert.CreateInvalidCharException` produces (`XmlConvert.cs:1614-1622`), with .NET's text:
69+
*"'…', hexadecimal value 0x01, is an invalid character."*
70+
71+
## 5. What is still open, and it is stated rather than left implicit
72+
73+
`XmlReaderSettings::CheckCharacters` also defaults to `true` and is also unenforced. This port's
74+
reader **accepts** these characters, so writing and reading now disagree — options A, B and D all
75+
leave that asymmetry and only C and E avoid it. .NET has no such asymmetry, because its reader
76+
enforces too.
77+
78+
The reader here is tinyxml2, which this port does not drive character by character, so closing it
79+
is not a matter of adding a call. It is recorded here rather than silently tolerated.
80+
81+
## 6. To migrate
82+
83+
A document containing these characters was never valid XML — the `Char` production excludes them
84+
and no character reference can carry them either, since a reference must itself match `Char`.
85+
Strip or replace them before writing.
86+
87+
## 7. Evidence
88+
89+
| Mutation | Caught |
90+
|---|---|
91+
| The check is byte-wise again (the shipped `VerifyXmlChars` shape) | ✅ (both door families) |
92+
| `U+FFFE` and `U+FFFF` become `Char` | ✅ (both) |
93+
| Tab, LF and CR stop being `Char` | ✅ (4 tests, including pre-existing ones) |
94+
| The C0 controls become `Char` (the pre-#2349 behaviour) | ✅ (both) |
95+
96+
## 8. Downstream
97+
98+
Neither `cna` nor `mobile-eggbert` references `XmlWriter`, `XDocument` or `XElement` — zero sites
99+
in both.

modules/xml-linq/include/System/Xml/Linq/detail/XLinqSerializationGuards.hpp

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include "System/Xml/XmlException.hpp"
88
#include "System/Xml/detail/XmlLexicalSanitizer.hpp"
9+
#include "System/ArgumentException.hpp"
910

1011
namespace System::Xml::Linq::detail {
1112

@@ -32,12 +33,12 @@ namespace System::Xml::Linq::detail {
3233
* why `System::Xml::detail::ContainsNul` (the single detector, shared with the writer door)
3334
* says so in its own doc-comment.
3435
*
35-
* **What this deliberately does NOT decide.** Characters outside `Char` other than NUL
36-
* (0x01, 0x0C, 0x0E–0x1F) are a different question: both doors emit them faithfully rather
37-
* than losing them, this runtime's own reader accepts them, and both "reject" and "emit" are
38-
* implementable — so whether they are rejected is a `XmlWriterSettings::CheckCharacters`
39-
* decision tracked separately. A flag can only govern a choice whose branches both exist.
40-
* This guard tests for a NUL and nothing else.
36+
* **The other non-`Char` characters, settled by #2349.** They were a different question when
37+
* this comment was written; the reference settles it. .NET's Xml.Linq save path builds a
38+
* DEFAULT `XmlWriterSettings` and touches only `Indent` and `NamespaceHandling`
39+
* (`XNode.cs:681-687`), so it inherits `CheckCharacters = true` and checks exactly as any
40+
* default-settings writer does. That is why this door needs no settings channel of its own,
41+
* and why the two doors agree by construction rather than by coordination.
4142
*
4243
* The diagnostic deliberately mirrors `XmlWriter.cpp`'s file-local `ThrowIfContainsNul`, so
4344
* the two doors of one node kind report the same cause under different member names.
@@ -55,6 +56,19 @@ namespace System::Xml::Linq::detail {
5556
throw System::Xml::XmlException(std::string(member) + ": the " + what +
5657
" contains a NUL character, which cannot be "
5758
"represented in XML.");
59+
60+
// #2349, the same check the writer door runs and for the same reason. The exception TYPE
61+
// is .NET's ArgumentException, not this door's XmlException: the NUL case above is this
62+
// port's own truncation guard (#2085) while this one transcribes
63+
// XmlConvert.CreateInvalidCharException (XmlConvert.cs:1614-1622).
64+
const std::size_t bad = System::Xml::detail::FindNonCharCodePoint(text);
65+
if (bad != std::string::npos) {
66+
std::uint32_t cp = 0;
67+
std::size_t len = 0;
68+
if (!System::detail::TryDecodeUtf8Scalar(text, bad, cp, len))
69+
cp = static_cast<unsigned char>(text[bad]);
70+
throw System::ArgumentException(System::Xml::detail::InvalidCharacterMessage(cp));
71+
}
5872
}
5973

6074
} // namespace System::Xml::Linq::detail

modules/xml-linq/tests/System/Xml/Linq/XLinqNulRejectionTests.cpp

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
#include <string>
5353

5454
#include "System/Xml/Linq/SaveOptions.hpp"
55+
#include "System/ArgumentException.hpp"
5556
#include "System/Xml/Linq/XAttribute.hpp"
5657
#include "System/Xml/Linq/XCData.hpp"
5758
#include "System/Xml/Linq/XComment.hpp"
@@ -334,20 +335,32 @@ TEST(XLinqNulRejectionTests, EmptyValuesAreStillAccepted) {
334335
EXPECT_EQ(direct(XComment("")), "<!---->");
335336
}
336337

337-
TEST(XLinqNulRejectionTests, NonNulControlCharacters_StillEmittedByTheDirectDoor) {
338-
// SCOPE BOUNDARY, and the mirror of the pin #2085 left at the writer door. 0x01, 0x0C and
339-
// 0x1F are outside the XML `Char` production but are EMITTED faithfully rather than lost,
340-
// and this runtime's own reader accepts them -- so both "reject" and "emit" are implementable
341-
// and choosing between them is the XmlWriterSettings::CheckCharacters decision, tracked
342-
// separately. #2201 deliberately does NOT make it. If a later ticket decides to reject them,
343-
// it updates this pin rather than deleting it.
344-
for (const char* v : {"a\x01" "b", "a\x0c" "b", "a\x1f" "b"}) {
345-
std::string value(v);
346-
EXPECT_NO_THROW((void)direct(XText(value))) << "0x" << static_cast<int>(value[1]);
347-
EXPECT_EQ(direct(XText(value)), value);
348-
EXPECT_NO_THROW((void)direct(XComment(value)));
349-
EXPECT_NO_THROW((void)XAttribute(XName("a"), value).ToString());
338+
// FLIPPED by #2349 (2026-08-18), the mirror of the writer door's pin flipping in the same change.
339+
// #2201 deliberately did not make the CheckCharacters decision; the reference makes it.
340+
//
341+
// AND IT DISSOLVES THE REASON THIS DOOR WAS THOUGHT TO BE DIFFERENT. #2349 priced option B as
342+
// "not a same-shaped change on both sides", because XNode::SerializeTo takes no settings.
343+
// .NET's does not either: XNode.GetXmlWriterSettings builds a DEFAULT XmlWriterSettings and
344+
// touches only Indent and NamespaceHandling (XNode.cs:681-687), inheriting CheckCharacters = true.
345+
// So this door needs no settings channel of its own, and the two agree by construction.
346+
TEST(XLinqNulRejectionTests, Fix2349_NonCharCodePointsAreRejectedAtTheDirectDoorToo) {
347+
for (const char* v : {"a\x01" "b", "a\x0c" "b", "a\x1f" "b", "a\x0b" "b"}) {
348+
const std::string value(v);
349+
EXPECT_THROW((void)direct(XText(value)), System::ArgumentException)
350+
<< "0x" << static_cast<int>(value[1]);
351+
EXPECT_THROW((void)direct(XComment(value)), System::ArgumentException);
352+
EXPECT_THROW((void)XAttribute(XName("a"), value).ToString(), System::ArgumentException);
353+
}
354+
355+
// Code points, not bytes: U+FFFE, U+FFFF and a lone surrogate are outside Char too, and a
356+
// byte-wise check accepts all three.
357+
for (const char* utf8 : {"\xEF\xBF\xBE", "\xEF\xBF\xBF", "\xED\xA0\x80"}) {
358+
EXPECT_THROW((void)direct(XText(std::string(utf8))), System::ArgumentException) << utf8;
350359
}
360+
361+
// Tab, LF and CR are Char and still pass, and so does ordinary multi-byte text.
362+
EXPECT_NO_THROW((void)direct(XText(std::string("a\tb\nc\rd"))));
363+
EXPECT_EQ(direct(XText(std::string("h\xC3\xA9llo"))), "h\xC3\xA9llo");
351364
}
352365

353366
TEST(XLinqNulRejectionTests, ConstructionAndMutationStillAcceptANul) {

modules/xml/include/System/Xml/detail/XmlLexicalSanitizer.hpp

Lines changed: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
55
#include <string>
6+
#include "System/detail/Utf8Scalar.hpp"
7+
#include <cstdint>
8+
#include <cstdio>
69

710
namespace System::Xml::detail {
811

@@ -166,14 +169,75 @@ namespace System::Xml::detail {
166169
* document; "write it in full" is not an implementable branch. This runtime's own parser
167170
* agrees, rejecting an embedded NUL in text with `XML_ERROR_PARSING_TEXT`.
168171
*
169-
* Other characters outside `Char` (0x01, 0x0C, ...) are a **different** question and are
170-
* deliberately not covered here: they are emitted faithfully rather than lost, this
171-
* runtime's own reader accepts them, and both "reject" and "emit" are implementable -- so
172-
* whether they are rejected is a `XmlWriterSettings::CheckCharacters` decision, tracked
173-
* separately. A flag can only govern a choice whose branches both exist.
172+
* Other characters outside `Char` (0x01, 0x0C, ...) were a **different** question, settled by
173+
* ticket #2349 -- see FindNonCharCodePoint below.
174174
*/
175175
[[nodiscard]] inline bool ContainsNul(const std::string& text) {
176176
return text.find('\0') != std::string::npos;
177177
}
178178

179+
/**
180+
* @brief Finds the first code point in @p text outside the XML 1.0 `Char` production.
181+
*
182+
* Ticket #2349 (2026-08-18), split from #2085. Characters outside `Char` other than NUL --
183+
* `0x01`, `0x0C`, `0x0E`-`0x1F`, `U+FFFE`, `U+FFFF` -- were **emitted raw** at every writer
184+
* content door and at every Xml.Linq direct door, so the emitted document was not
185+
* well-formed XML. Measured: 28 of the 29 non-`Char` bytes in `0x00`-`0x1F` went through.
186+
*
187+
* **THE TICKET RECORDED THIS AS A USER DECISION WITH FIVE PRICED OPTIONS, AND THE REFERENCE
188+
* COLLAPSES THEM TO ONE.** `XmlWriterSettings.CheckCharacters` defaults to `true`
189+
* (`XmlWriterSettings.cs:513`) and is enforced: `XmlEncodedRawTextWriter.InvalidXmlChar`
190+
* throws `XmlConvert.CreateInvalidCharException` when it is set and entitizes when it is not
191+
* (`:1630-1654`). So .NET rejects by default, and the flag is what turns that off.
192+
*
193+
* **Both of the ticket's pricing complications are dissolved rather than accepted.**
194+
*
195+
* 1. It priced enforcement on `XmlConvert::VerifyXmlChars`, which iterates `char` and so
196+
* checks **bytes**: it accepts `U+FFFE`, `U+FFFF` and a lone surrogate encoded in UTF-8.
197+
* Ticket **#2354** moved a code-point decoder into `Core.Base` earlier the same day, and
198+
* both `modules/xml` and `modules/xml-linq` already depend on it -- so a code-point
199+
* correct check costs one call and no new component edge. The ticket priced this when
200+
* that decoder was not there.
201+
* 2. It priced option B as "not a same-shaped change on both sides", because the Xml.Linq
202+
* direct doors take no settings. **.NET's do not either**: `XNode.GetXmlWriterSettings`
203+
* constructs a default `XmlWriterSettings` and touches only `Indent` and
204+
* `NamespaceHandling` (`XNode.cs:681-687`), inheriting `CheckCharacters = true`. So the
205+
* Linq side needs no new settings channel; it checks, exactly as a default-settings
206+
* writer does, and the two doors agree by construction.
207+
*
208+
* An **ill-formed UTF-8 sequence** is rejected too, and that is not an extension: .NET works
209+
* in UTF-16 and a lone surrogate is outside `Char` there as surely as it is here.
210+
*
211+
* @return The offending code point's byte offset, or `std::string::npos` when every code
212+
* point is a `Char`.
213+
*/
214+
[[nodiscard]] inline std::size_t FindNonCharCodePoint(const std::string& text) {
215+
std::size_t i = 0;
216+
while (i < text.size()) {
217+
std::uint32_t cp = 0;
218+
std::size_t len = 0;
219+
if (!System::detail::TryDecodeUtf8Scalar(text, i, cp, len)) return i;
220+
// Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF].
221+
// The decoder has already refused a surrogate and anything above U+10FFFF, so what is
222+
// left to exclude is the C0 controls other than tab/LF/CR, and the two non-characters
223+
// at the end of the BMP.
224+
const bool isChar = cp == 0x09 || cp == 0x0A || cp == 0x0D ||
225+
(cp >= 0x20 && cp <= 0xD7FF) ||
226+
(cp >= 0xE000 && cp <= 0xFFFD) ||
227+
(cp >= 0x10000 && cp <= 0x10FFFF);
228+
if (!isChar) return i;
229+
i += len;
230+
}
231+
return std::string::npos;
232+
}
233+
234+
/** @brief `.NET`'s `SR.Xml_InvalidCharacter`, formatted for @p codePoint. */
235+
[[nodiscard]] inline std::string InvalidCharacterMessage(std::uint32_t codePoint) {
236+
char hex[16] = {};
237+
std::snprintf(hex, sizeof(hex), "0x%02X", codePoint);
238+
std::string rendered;
239+
if (codePoint >= 0x20 && codePoint < 0x7F) rendered.push_back(static_cast<char>(codePoint));
240+
return "'" + rendered + "', hexadecimal value " + hex + ", is an invalid character.";
241+
}
242+
179243
} // namespace System::Xml::detail

modules/xml/src/System/Xml/XmlWriter.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,22 @@ static void ThrowIfContainsNul(const std::string& text, const char* member, cons
5353
if (detail::ContainsNul(text))
5454
throw XmlException(std::string("XmlWriter::") + member + ": the " + what +
5555
" contains a NUL character, which cannot be represented in XML.");
56+
57+
// Ticket #2349. The other characters outside the XML 1.0 Char production were emitted RAW,
58+
// so the emitted document was not well-formed XML. .NET rejects them when
59+
// XmlWriterSettings.CheckCharacters is set, which it is by default
60+
// (XmlWriterSettings.cs:513; XmlEncodedRawTextWriter.cs:1630-1654). The exception TYPE is
61+
// .NET's ArgumentException rather than this door's XmlException, because that is what
62+
// XmlConvert.CreateInvalidCharException produces (XmlConvert.cs:1614-1622) -- the NUL case
63+
// above keeps XmlException because it is this port's own truncation guard (#2085), not a
64+
// transcription of a .NET check.
65+
const std::size_t bad = detail::FindNonCharCodePoint(text);
66+
if (bad != std::string::npos) {
67+
std::uint32_t cp = 0;
68+
std::size_t len = 0;
69+
if (!System::detail::TryDecodeUtf8Scalar(text, bad, cp, len)) cp = static_cast<unsigned char>(text[bad]);
70+
throw System::ArgumentException(detail::InvalidCharacterMessage(cp));
71+
}
5672
}
5773

5874
static void ThrowIfClosed(const XmlWriterState* state, const char* member) {

0 commit comments

Comments
 (0)