Page MenuHomeWildfire Games

Validate XML directory and file attributes strings when loading.
ClosedPublic

Authored by nani on Oct 9 2019, 5:05 PM.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nani created this revision.Oct 9 2019, 5:05 PM
elexis added a subscriber: elexis.Oct 11 2019, 7:39 PM
elexis added inline comments.
source/gui/CGUI.cpp
941 ↗(On Diff #10117)

Could it happen here too that it silently ignores file loading under any circumstance?

961 ↗(On Diff #10117)

The try/catch block looks like it should be removed, see D2366.

elexis requested changes to this revision.Oct 11 2019, 9:33 PM
elexis added inline comments.
source/gui/CGUI.cpp
941 ↗(On Diff #10117)

Something similar can be triggered for this case:

<script file="gui/gamesetup/"/>

vfs_lookup.cpp(141): Assertion failed: "!pathname.IsDirectory()"
Assertion failed: "!pathname.IsDirectory()"
Location: vfs_lookup.cpp:141 (vfs_Lookup)

The rationale is that one can pass any JS and XML to the engine but won't be able to crash it.

961 ↗(On Diff #10117)

Done in rP23068

This revision now requires changes to proceed.Oct 11 2019, 9:33 PM
nani updated this revision to Diff 10126.Oct 11 2019, 10:01 PM
nani updated this revision to Diff 10127.Oct 11 2019, 10:16 PM

Added check for file attribute.

nani updated this revision to Diff 10129.Oct 11 2019, 10:38 PM

See PM discussion yesterday, this will only catch the case where the directory doesnt end with a slash, but in case someone has a typo in the foldername, it will not report that and continue like nothing happened, just with follow up errors.

There was also a discussion about the ENSURE in VfsDirectoryExists from rP16773.

An ENSURE means if the condition is not met, it is an error. by the C++ author. If it is an error by the JS or XML author, then it should use LOGERROR or JS_ReportError.

It appears the VfsDirectoryExists function is currently only used for hardcoded folders. This would be the first use case where user-input (JS/XML) is fed.

As nani indicated, the ENSURE may be moved to the callers of that function too.

(One could also wonder whether its a feature that 0 files being loaded is going unreported, so that mods could delete entire directories without overwriting the directory including including XML pagefile.
But usually if they want to delete a folder, they have to change much more already. And the XML files should also be split so that changing the includes is more friendly for combining mods (for example 0ad + fgod + autociv).)

nani retitled this revision from Check attribute directory path is actually a directory when loading the folder's scripts. to Validate XML directory and file attributes strings when loading..Nov 18 2019, 4:43 PM
elexis accepted this revision.Jan 20 2020, 10:53 AM

The patch results in

ERROR: GUI: Script path gui/pregame is not a directory

while it actually is a directory, just not one recognized by the code.

At least from a perspective of a modder who doesnt know anything about the C++ code, he will wonder whats wrong, and doesnt get a clue that the trailing slash is missing.

I will add the word "path", hopefully making that more clear, as I think the addition of "did you forget to add a /?" would give a wrong hint in case there was a typo for example or a filepath given.

ERROR: GUI: Script path gui/pregame is not a directory path

Notice that this function actually only checks whether the string is nonempty and ends with slash, but doesnt check if its a directory or file nor if it exists.
The latter cases are covered by a VFS logerror, therefore the patch is complete.

The file check is useful because it means a logerror is shown instead of the application crashing / running into a debug breakpoint.

Thanks for the patch nani.

This revision is now accepted and ready to land.Jan 20 2020, 10:53 AM