Skip to content

Preserve illegal XML characters as character references - #6767

Open
kalayciburak wants to merge 1 commit into
apache:masterfrom
kalayciburak:fix/6761-woodstox-invalid-xml-chars
Open

kalayciburak wants to merge 1 commit into
apache:masterfrom
kalayciburak:fix/6761-woodstox-invalid-xml-chars

Conversation

@kalayciburak

@kalayciburak kalayciburak commented Sep 21, 2026 •

Copy link
Copy Markdown

Description

Write characters that XML 1.0 does not allow as character references, and accept those references on read.

JMeter 5.6.3 wrote NUL and other C0 controls as references (�, ) and read them back. After the Woodstox migration the save throws. Replacing the character with a space would drop the original bytes. This keeps the 5.6.3 round trip: save writes the reference, and load accepts �,  and , including files already written by 5.6.3.

JMeterStaxDriver uses WstxOutputFactory and WstxInputFactory directly, so a plugin StAX jar or a javax.xml.stream system property cannot replace them.

Motivation and Context

Fixes #6761

How Has This Been Tested?

  • ./gradlew :src:core:test --tests org.apache.jmeter.save.SaveServiceInvalidXmlCharTest 17/0
  • ./gradlew :src:core:checkstyleMain :src:core:checkstyleTest

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@vlsi vlsi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Saving no longer throws, but the fix loses data without any warning, and it leaves the load side of #6761 broken.

  1. Saved values no longer survive save and reopen. JMeter 5.6.3 wrote these characters as character references (XppDriver uses XStream's PrettyPrintWriter in XML_QUIRKS mode, which emits � and ) and read them back, so a recorded binary body was preserved. With this PR, new StringProperty("bin", "pre\u0000mid\u001fsuf") saved with SaveService.saveElement and read with SaveService.loadElement comes back as "pre mid suf". A plan with a recorded binary body sends different bytes after Save, and nothing is logged. The Woodstox migration is unreleased, so 5.6.3 is the baseline users compare against.
  2. Files saved by 5.6.3 still cannot be opened. Reading goes through Woodstox too, and it rejects the references 5.6.3 wrote: � fails with Invalid character reference: null character not allowed in XML content, and  fails with Illegal character entity: expansion character (code 0x1f). The issue attaches such a file (01-recording-test-5_6_3.jmx). Please cover loading as well. One option that restores 5.6.3 behavior in both directions: write these characters as character references and accept them on read.
  3. U+FFFE still produces a file that cannot be loaded. Woodstox writes  without complaint, and loading it fails with Illegal character entity: expansion character (code 0xfffe). The description says the replacement keeps the XML well-formed, but that holds only for the C0 range.
  4. The user manual describes the old format. The note in xdocs/usermanual/component_reference.xml ("XML files written by JMeter have version 1.0 declared in header while actual file is serialized with 1.1 rules") no longer matches what JMeter writes. Please update it to the behavior this PR settles on.

override fun createOutputFactory(): XMLOutputFactory =
XMLOutputFactoryDelegate(super.createOutputFactory(), xmlHeader = xmlHeader, indent = indent)
override fun createOutputFactory(): XMLOutputFactory {
val factory = super.createOutputFactory()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

StaxDriver.createOutputFactory() returns XMLOutputFactory.newInstance(), which is not necessarily Woodstox. A plugin in lib/ext that ships another StAX implementation (Aalto, for instance), or a javax.xml.stream.XMLOutputFactory system property, selects a different factory. setProperty with a Woodstox-only key then throws IllegalArgumentException, and every save fails. Please create WstxOutputFactory explicitly, or check isPropertySupported before setting the property.

Comment on lines +31 to +32
// Woodstox rejects XML 1.0-illegal characters (NUL, C0 controls) by default.
// Replace them so JTL/JMX save of binary payloads does not fail (#6761).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

NUL is itself a C0 control, and tab, line feed, and carriage return are C0 controls that XML 1.0 allows, so "NUL, C0 controls" names a larger set than the one Woodstox rejects. The comment also leaves out the consequence a maintainer needs: the replacement cannot be undone, and it applies to attribute values as well as text. For the current approach, something like:

// Woodstox rejects C0 control characters other than tab, LF, and CR.
// Each one is written as a space, in text and in attribute values, so the original character is lost.

Comment on lines +33 to +36
factory.setProperty(
WstxOutputProperties.P_OUTPUT_INVALID_CHAR_HANDLER,
InvalidCharHandler.ReplacingHandler(INVALID_XML_CHAR_REPLACEMENT)
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The handler replaces characters in attribute values too: a sample label "lab\u0002el" is written to the JTL as lb="lab el". Please cover that in a test and in the changelog, whichever behavior the rework settles on.

Comment on lines +40 to +42
private companion object {
const val INVALID_XML_CHAR_REPLACEMENT = ' '
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A companion object for one private Char is more structure than the value needs. A file-level private const val does the same.

Comment on lines +34 to +37
/**
* Regression for <a href="https://github.com/apache/jmeter/issues/6761">#6761</a>:
* Woodstox rejects characters that are illegal in XML 1.0 when saving JTL/JMX.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The comment opens with the defect in the present tense, and after the fix it is no longer true. State the rule the test checks first, then the old defect in one past-tense sentence. With the current behavior, for example:

/**
 * A character that XML 1.0 does not allow is saved as a space.
 * Saving such a value used to fail with WstxIOException (issue #6761).
 */

Comment on lines +53 to +58
assertDoesNotThrow(() -> SaveService.saveSampleResult(new SampleEvent(result, "tg"), writer));

String xml = writer.toString();
assertFalse(xml.isEmpty(), "JTL output should not be empty");
assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in well-formed XML");
assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not appear in XML 1.0");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These assertions still pass if the value is dropped entirely: the output is non-empty and contains no NUL. Please assert the literal that is expected in the output, e.g. <samplerData class="java.lang.String">pre mid suf</samplerData>, so the test fails when the written value is wrong.

assertFalse(xml.indexOf(..) >= 0) prints only expected: <false> but was: <true> on failure. An assertEquals against the expected string prints both values. Once the test asserts the output, assertDoesNotThrow adds nothing.

String xml = writer.toString();
assertFalse(xml.isEmpty(), "JTL output should not be empty");
assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in well-formed XML");
assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not appear in XML 1.0");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

0x1F is a C0 control character, not C1 (C1 is 0x80–0x9F). The same message is on line 70.

Comment on lines +61 to +71
@Test
void saveElementAllowsNulInStringProperty() {
StringProperty property = new StringProperty("bin", ILLEGAL_XML_CHARS);
ByteArrayOutputStream out = new ByteArrayOutputStream();
assertDoesNotThrow(() -> SaveService.saveElement(property, out));

String xml = out.toString(StandardCharsets.UTF_8);
assertFalse(xml.isEmpty(), "JMX fragment should not be empty");
assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in well-formed XML");
assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not appear in XML 1.0");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What users need is that a plan saved with such a value opens again with a known value. Please add:

  • a round-trip test: saveElement, then SaveService.loadElement, asserting the loaded value;
  • a test that loads a 5.6.3-style fragment containing &#x0; and &#x1f;, which fails on the current code.

The name says Nul, but the input also contains 0x1F, and with both characters in one string no test checks 0x1F on its own, although it is the character in the first stack trace of the issue. A parameterized test with one case per character, plus a case for a character in an attribute (the sample label), would cover each separately.

Comment thread xdocs/changes.xml Outdated
<li><issue>5937</issue>Remove deprecated Log4j package scanning and configure plugin metadata processing to improve startup time and avoid deprecation warnings. Contributed by Piotr P. Karwasz (github.com/piotrgithub)</li>
<li><pr>6620</pr>Fix report generation paths so dashboard output files are created in the correct location after internal refactoring.</li>
<li><bug>6456</bug>Handle malformed percent-encoded URLs gracefully when recording HTTP traffic, logging a warning instead of failing the recording.</li>
<li><issue>6761</issue>Allow XML save of sample results and test plans that contain characters illegal in XML 1.0 (NUL and C0 controls) after the Woodstox migration.</li>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The Woodstox migration has not been released, so users upgrading from 5.6.3 never saw this failure, and "after the Woodstox migration" describes internal history. For those users, the visible change is what happens to these characters on save: replaced with a space, or kept, depending on the rework. Please describe that instead.

Woodstox rejects NUL and other characters that XML 1.0 does not allow.
Write them as character references and accept those references on read,
so save and reload keep the original value, including files written by
JMeter 5.6.3.

Closes apache#6761
@kalayciburak
kalayciburak force-pushed the fix/6761-woodstox-invalid-xml-chars branch from 29cc466 to 57377f0 Compare September 24, 2026 21:40
@kalayciburak kalayciburak changed the title Don't fail XML save when sample data contains illegal XML characters Preserve illegal XML characters as character references Sep 24, 2026
@kalayciburak

Copy link
Copy Markdown
Author

switched to character references, same as 5.6.3, so save and reload keep the value. 5.6.3 files with � and � load again. pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants