If an XML file happens to have syntax error (such as the one in credentials.xml in rP21847), then CXeromyces::ConvertFile will parse that file for the first time, throw a syntax error and then save a cached version of that file that doesn't have a syntax error to the cache.
Further loads will then go for the valid cache file, hiding the syntax error and possibly misleading authors into assuming their XML files are valid (such as in that commit).
Details
- Reviewers
bb - Commits
- rP22921: Reliably report and reject invalid XML files following rP16733, refs #245…
- Trac Tickets
- #5222
Correctness: Notice that not writing a syntactically valid (but possibly semnatically wrong) cache file meets the only requirement of the claimed task.
Completeness: Judge whether this can break under some edge cases, such as packaged mods.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 9424 Build 15686: Vulcan Build Jenkins Build 15685: Vulcan Build (Windows) Build 15684: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/644/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/645/display/redirect
Agreeing
If it would crash for mods, the mods would receive an error in the current code, thus not caching the file wouldn't change much there.
source/ps/XML/Xeromyces.cpp | ||
---|---|---|
177 | I think it's better to call it validSource, because of the function result is xmb. |
ArchiveBuilder.cpp runs GenerateCachedXMB, which calls ConvertFile, which should the LOGERROR.
If the LOGERROR does not show up when running the archive build, that will be an important independent bug to solve.
mod bundlers already should see the error when they budnle, with this patch they will also see it more than once when running the affected code.
Since the cached file is not written anymore, the bundling script will exclude the XMB file for the invalid XML file.
That however is analogous to the other two LOGERRORs upon syntax errors.
The archive builder has an ENSURE that is triggered for the other LOGERRORs however, it's not triggered for this one.
So it seems like a return value would be useful.
It even says so: // For now, log the error and continue, in the future we might fail.
There is the same code in CXeromyces::LoadString.
...
Spent too much time trying to create a testcase, but it doesnt seem possible without changing the existing code,
mostly due to the static global defined in the implementation file that is not accessible.
source/ps/XML/Xeromyces.cpp | ||
---|---|---|
177 | (Guess its not so ambiguous since valid refers to the process of validation, but if we want to narrow it down, I'd suggest XML, as that's even more narrow than source) |
source/ps/XML/Xeromyces.cpp | ||
---|---|---|
177 | Actually validXML is wrong, becaues the XML is valid according to XML specs, but the XML is not valid for that grammar. |
Return error code as well, i.e. doesnt load the file that is valid XML and also making the archive builder fail instead of silently accepting the file, like it does with XML files with (other) syntax errors.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/ps/XML/Xeromyces.h | 1| /*·Copyright·(C)·2015·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2015" source/ps/XML/Xeromyces.h | 115| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file. | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classXMBFile{' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/726/display/redirect