Page MenuHomeWildfire Games

Change Vfs_Attach order so that mods can't overwrite config/ or screenshots/ paths
Needs ReviewPublic

Authored by wraitii on Sep 2 2019, 9:31 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

As discovered with elexis on IRC today (2019-09-02), mods can overwrite config files, including default.cfg.
This seems wrong, because there are plenty of comments saying this should be impossible (see e.g. GameSetup.cpp:L461)

I've looked around a lot... This is a bit nebulous. This code changed quite a bit over the years, but it seems as far as I can tell that it hasn't worked properly for at least years.

However, vfs_attack does something weird: it first populates a directory, then sets its 'real directory' counterpart.
It would seem much more logical to do the opposite. It comes right away from rP6333, and it seems like the ordering was perhaps a mistake?

This seems to do the correct thing anyways and mods aren't broken.

Test Plan

Agree that this is a fix.

Event Timeline

wraitii created this revision.Sep 2 2019, 9:31 PM
Vulcan added a comment.Sep 2 2019, 9:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/54/display/redirect

Vulcan added a comment.Sep 2 2019, 9:38 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/file/vfs/vfs_populate.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/563/display/redirect

Submerged again in VFS for a while, and I'm certain that this change is correct. It makes no sense to populate without changing the real directory first, since that's what gets used for populating, and it leads to the above user.cfg issue (basically by being a data-race).

However I think mods shouldn't be able to run into conflicts with other 'namespaces', and thus all mods should be put in a child 'mods' directory or the VFS, not at root, to be safer.

The 'data-race' is the following.
First, some preliminaries:

  • vfs_Lookup returns a VFSDirectory. It may or may not skip populating. When Mount() is called, it does skip populating. There is a 1-1 correspondance between a Vfs Path (e.g. "config/") and a VfsDirectory. If you mount multiple real folders to a single VfsPath, then there is still only one directory that gets loaded with all these files. The whole point of vfsLookup is to ensure this 1-1 correspondence.
  • vfs_Populate calls vfs_attach on subdirectories.
  • vfs_Attach populates the Directory (== vfs Path), if needed, with the content of the current real directory, then updates the real directory and says that a 'populate' call is needed [NB: it should be noticed from reading this sentence that this is odd].
  1. Mount a mod A, containing a config folder, to L"".
    • At this point, the root directory points to the mod's directory, and nothing else exists in the VFS since it wasn't populated <=> L"" needs populating.
  2. Mount the user's config folder to L"config/".
    • A L"config/" VfsDirectory is created, unpopulated. It points to the user config. both L""and L"config/" need populating.
  3. Access some other file from the mod, such as L"simulation/components/Health.js".
    1. VFS_Lookup starts at L"", the root directory, and notices that it must be populated.
    2. Per the above, vfs_attach thus gets called for L"config/" with the mod config folder. It thus populates with the content of the real directory, which is still the user config folder. It updates L"config/" to point to the mod config folder. L"config/" needs populating and points to the mod config folder - which shouldn't happened.

If one swaps steps 3 and 2, then the mod A no longer needs populating, and `L"config/" correctly points to the user config. This is why I call this a 'data race'.

The patch above fixes this, because it makes vfs_attach populate with the updated real directory right away. This is fairly coincidental, but it makes much more sense for vfs_attach to do this than to populate with the content of the last-specified-real-directory.


On a separate note, I feel like this whole logic of sometimes populating and sometimes not in vfs_lookup is odd.

Last thing I wanted to check was whether this would slow down loading times, but we don't populate from archives anyways and we have to populate at some point if files are to exist, so that ought to be irrelevant.