Preserve illegal XML characters as character references - #6767
kalayciburak wants to merge 1 commit into
Conversation
vlsi
left a comment
There was a problem hiding this comment.
Saving no longer throws, but the fix loses data without any warning, and it leaves the load side of #6761 broken.
- Saved values no longer survive save and reopen. JMeter 5.6.3 wrote these characters as character references (
XppDriveruses XStream'sPrettyPrintWriterinXML_QUIRKSmode, which emits�and) and read them back, so a recorded binary body was preserved. With this PR,new StringProperty("bin", "pre\u0000mid\u001fsuf")saved withSaveService.saveElementand read withSaveService.loadElementcomes 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. - 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 withInvalid character reference: null character not allowed in XML content, andfails withIllegal 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. U+FFFEstill produces a file that cannot be loaded. Woodstox writeswithout complaint, and loading it fails withIllegal character entity: expansion character (code 0xfffe). The description says the replacement keeps the XML well-formed, but that holds only for the C0 range.- 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() |
There was a problem hiding this comment.
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.
| // 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). |
There was a problem hiding this comment.
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.| factory.setProperty( | ||
| WstxOutputProperties.P_OUTPUT_INVALID_CHAR_HANDLER, | ||
| InvalidCharHandler.ReplacingHandler(INVALID_XML_CHAR_REPLACEMENT) | ||
| ) |
There was a problem hiding this comment.
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.
| private companion object { | ||
| const val INVALID_XML_CHAR_REPLACEMENT = ' ' | ||
| } |
There was a problem hiding this comment.
A companion object for one private Char is more structure than the value needs. A file-level private const val does the same.
| /** | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
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).
*/| 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"); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
0x1F is a C0 control character, not C1 (C1 is 0x80–0x9F). The same message is on line 70.
| @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"); | ||
| } |
There was a problem hiding this comment.
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, thenSaveService.loadElement, asserting the loaded value; - a test that loads a 5.6.3-style fragment containing
�and, 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.
| <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> |
There was a problem hiding this comment.
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
29cc466 to
57377f0
Compare
|
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. |
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.JMeterStaxDriverusesWstxOutputFactoryandWstxInputFactorydirectly, so a plugin StAX jar or ajavax.xml.streamsystem property cannot replace them.Motivation and Context
Fixes #6761
How Has This Been Tested?
./gradlew :src:core:test --tests org.apache.jmeter.save.SaveServiceInvalidXmlCharTest17/0./gradlew :src:core:checkstyleMain :src:core:checkstyleTestTypes of changes
Checklist: