Repository navigation
Fix #15107: Exporting XML via GUI escapes special characters twice. - #8943
ludviggunne wants to merge 2 commits into
Conversation
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The writer no longer escapes twice, but the reader still unescapes twice. XmlReportV2::writeError() now relies on QXmlStreamWriter's escaping alone. XmlReportV2::readError() still runs XmlReport::unquoteMessage() on msg, verbose, file, file0 and info after QXmlStreamReader has already decoded them. Any text that contains a literal entity such as <, &, " or ' therefore changes on a save/load round trip. Before this PR the round trip kept the text intact, because the extra quoting and unquoting cancelled out.
Cppcheck produces such messages itself. if (strcmp(s, "<") == 0 && strcmp(s, "<") == 0) gives Condition 'strcmp(s,"<")==0' is always true. After saving this from the GUI (or after it is written to the project's lastResults.xml) and loading it again, the message reads strcmp(s,"<"). ErrorItem::same() compares message and errorPath when there is no hash, so these items can also lose their sinceDate and tags in updateFromOldReport.
To keep reading old double-escaped files, the unquoting should depend on the file, for example on a version or marker in the header that this PR's writer emits. It should not run on every file. The new test does not catch the problem because its strings contain no literal entity. The XML that the CLI writes was already decoded twice by the GUI before this PR; this PR adds the same problem for files the GUI writes itself.
Testing
- Qt is not installed in the container, so I could not build the GUI or run
gui/test/xmlreportv2. The problem is found by reading the code, but it is concrete. - Built
cppcheckandtestrunnerinpr/withmake -j8;./testrunner TestErrorLoggerpasses. The PR does not change the core library or the CLI. - Showed that cppcheck emits messages with literal entities:
// scratch/a.c
#include <string.h>
void f(const char *s) {
if (strcmp(s, "<") == 0 && strcmp(s, "<") == 0) {}
}$ pr/cppcheck --enable=all --xml a.c
<error id="knownConditionTrueFalse" ... msg="Condition 'strcmp(s,"&lt;")==0' is always true" ...>
<location file="a.c" line="3" column="53" info="Condition 'strcmp(s,"&lt;")==0' is always true"/>
- Round trip in the GUI, traced in the code. The message is
Condition 'strcmp(s,"<")==0' is always true.- With the PR, the writer stores
&lt;.QXmlStreamReaderdecodes that to<, andunquoteMessagethen turns it into<, so the reloaded message hasstrcmp(s,"<"). - On base, the writer stored
&amp;lt;. Reading decoded it to&lt;and unquoting gave<, so the message came back correctly.
- With the PR, the writer stores
- A version of the new test that would catch this: set
item.summary = "x < y", then checkQCOMPARE(errors[0].summary, item.summary)after reading.
Automated review of 54b2dde by Claude. The pull request was built and tested locally.
| mXmlWriter->writeAttribute(MsgAttribute, summary); | ||
| const QString message = XmlReport::quoteMessage(error.message); | ||
| mXmlWriter->writeAttribute(VerboseAttribute, message); | ||
| mXmlWriter->writeAttribute(MsgAttribute, error.summary); |
There was a problem hiding this comment.
AI review
Now that the attribute is escaped only once, the reader at line 230 still calls XmlReport::unquoteMessage() on the value that QXmlStreamReader has already decoded. Literal entities in a message are lost on save/load. For example, cppcheck reports Condition 'strcmp(s,"<")==0' is always true; it is written as &lt;, read back as < and then unquoted to <. Before this PR the round trip was correct. The same applies to file, file0 and info (lines 259/261/266). Fallback matching in ErrorItem::same() uses message and errorPath, so updateFromOldReport() on lastResults.xml can also stop matching these items. Old double-escaped files should be detected, for example from a marker or version in the header, and only those should be unquoted.
| * @brief Unquote the message. | ||
| * @param message Message to quote. | ||
| * @return quoted message. | ||
| * For older reports where special characters have been escaped twice (#15107). |
There was a problem hiding this comment.
AI review
This comment says the function is for older reports, but readError() calls it on every file. That includes files written after this PR and the XML that the CLI writes, which are escaped only once.
| item.errorId = "unusedVariable"; | ||
| item.severity = Severity::style; | ||
| item.summary = "Unused variable: \"x\" & <y>"; | ||
| item.message = item.summary; |
There was a problem hiding this comment.
AI review
The round-trip check passes only because the string has no literal entity. With e.g. item.summary = "x < y", the QCOMPARE(errors[0].summary, item.summary) below would fail, because unquoteMessage turns the text that was read into x < y.
2 participants
No description provided.