search

ModOrganizer2 / modorganizer

Mod manager for various PC games. Discord Server: https://discord.gg/ewUVAqyrQX if you would like to be more involved
http://www.nexusmods.com/skyrimspecialedition/mods/6194
GNU General Public License v3.0
2.05k stars 155 forks source link

[Cyberpunk 2077] Conflicts with VFS when using RedFileSystem plugin #2039

Open poirierlouis opened 3 weeks ago

poirierlouis commented 3 weeks ago

The problem:

I'm the author of the mod RedFileSystem which allow other modders to read/write files in what I call storage endpoints. Currently, when running Cyberpunk with RedFileSystem and HUDPainter it will not work when using MO2. Path is somehow mixed up between what RedFileSystem expect and what MO2 provides.

To Reproduce:

  1. Install RedFileSystem and HUDPainter along with their dependencies using MO2.
  2. Start the game.
  3. HUDPainter will show a warning to the user saying a preset file cannot be found.

Environment:

Details:

Hierarchy without MO2 is:

(1) <GAME>\r6\storages\               # The root directory RedFileSystem works with.
                                      # It is created when not present at startup.

(2) <GAME>\r6\storages\HUDPainter\    # Storage of mod HUDPainter provided by RedFileSystem.
                                      # It is created when not present at startup.

HUDPainter embed a file in its archive to serve to users:

(3) <GAME>\r6\storages\HUDPainter\DEFAULT.json

It introduces different mappings when RedFileSystem is used to read / write a file. It is also broken when RFS is used to list files of a storage (using directory_iterator):

So far we have been able to bring fixes in RedFileSystem. But, in this case, r6\storages\HUDPainter\DEFAULT.json will not be in <MO2>\overwrite\... but in <MO2>\mods\HUD Painter\.... My understanding is that when reading an existing file from an installed mod, path "send" to RFS will start with <MO2>\mods\[ModName]\... while writing a new file using RFS, path "send" to RFS will start with <MO2>\overwrite\....

See also this PR where we are discussing this issue.

I'd appreciate any advice and help to work this problem, in order to allow players to use RedFileSystem together with MO2.

More specifically, is it possible to detect/get if a path will be in <GAME>, <MO2>\overwrite or <MO2>\mods\[ModName]?

A proposal might be to update MO2 such as path r6\storages could be an exception?

Link to Mod Organizer logs:

I don't think it is relevant here, but let me know if you need it.

USVFS:

I don't think it is relevant here, but let me know if you need it.

MO Interface:

I don't think it is relevant here, but let me know if you need it.

Holt59 commented 3 weeks ago

Your problem is not clear to me - When launching anything through MO2, you should not care if it is in mods or in overwrites, the program should see everything in the game folder.

Can you clarify the actual issue with a minimal example, i.e., what files do you have in each mods, what files do you expect to see and at which location in the game and what you actually see?

poirierlouis commented 3 weeks ago

Sorry, I'll start again:

you should not care if it is in mods or in overwrites, the program should see everything in the game folder.

I was because RedFileSystem implement a security measure, which is supposed to prevent modders to access files outside of Cyberpunk 2077\r6\storages\[ModName]. To check it is legitimate, not in a parent directory, I use weakly_canonical to resolve the path. When this function is used, it returns the actual path of MO2. Which is different from the root path (of the game) I use to compare. That is to make sure beginning of the resolved path start with C:\...\Cyberpunk 2077\r6\storages\[ModName] too. And not something like C:\...\Steam\.

Here an example with HUD Painter installed. Directory is empty in:

<GAME>/r6/storages/

Archive of HUD Painter embeds a file:

<ZIP>/r6/storages/HUDPainter/DEFAULT.json

HUD Painter at startup request its storage endpoint in:

/r6/storages/HUDPainter

RFS tests if directory exists or not. First time, it doesn't exist. So RFS creates this directory (logs shows it works, no error from STL). But directory is not created in <GAME>/r6/storages/HUDPainter nor in <MO2>/overwrite/r6/storages/HUDPainter. Which is a weird behavior introduced by MO2. (bug A)

HUD Painter need to read the file (1), MO2 provides this path (2):

(1) /r6/storages/HUDPainter/DEFAULT.json
(2) <MO2>/mods/HUDPainter/r6/storages/HUDPainter/DEFAULT.json

HUD Painter write a new file (1), MO2 provides this path (2):

(1) /r6/storages/HUDPainter/TEST.json
(2) <MO2>/overwrite/r6/storages/HUDPainter/TEST.json

Because of bug A, directory doesn't exist, so file is not created.

When writing new file, I can use create_directories just-in-time. In such case, directory (2) is created and file is also created (fix bug A). But when HUD Painter request a list of files in /r6/storages/HUDPainter/, it will only list this path:

<GAME>/r6/storages/HUDPainter/DEFAULT.json

When starting the game again, it list both files as expected:

<GAME>/r6/storages/HUDPainter/DEFAULT.json
<GAME>/r6/storages/HUDPainter/TEST.json

Restarting the game is not convenient nor expected (bug B).

It looks like mapping of new directory is not taken into account by VFS of MO2. Bug A: can be fixed on my side Bug B: needs a fix on MO2 side

I hope it is explicit enough.

Holt59 commented 3 weeks ago

If I summarize what you're saying, MO2 does not return what you expect when using std::weakly_canonical? Could by chance create a very small C++ example that, when run from MO2, does not produce what you expect? That would help a lot debugging the problem.

poirierlouis commented 3 weeks ago

MO2 does not return what you expect when using std::weakly_canonical?

Yes, when using it for security purpose. But I can probably workaround this issue later. The main problem is described when not using std::weakly_canonical, for bug A and B.

Could by chance create a very small C++ example that, when run from MO2, does not produce what you expect?

I'll try to reproduce an example, with the same steps.

Al12rs commented 3 weeks ago

@poirierlouis The expected behaviour of running a program or tool through the MO2 Virtual File System, is that it would not be able to notice it is being virtualized or not.

So the unexpected behaviour here is the fact that std::weakly_canonical is returning the actual MO2 path, rather than the virtual path inside the game folder. This is breaking the virtualization.

RedFileSystem should be getting the virtual path inside the game folder, but it is getting the actual path.

Regarding Bug A and Bug B.

I think the issue is that RedFileSystem is trying to use the Actual path that you get returned by std::weakly_canonical (<MO2>/mods/HUDPainter/r6/storages/HUDPainter/DEFAULT.json).

By doing so RedFileSystem is bypassing the VFS, as we are not watching the MO2 Overwrite path, we are watching the game path.

The idea is that RedFileSystem should be getting back the virtual path of /r6/storages/HUDPainter from the first request of generating this folder. And it should continue to use this path (/r6/storages/HUDPainter) to then generate new files in it.

The VFS will not actually generate the empty folder in overwrite, but it should keep track of it in memory. So if you attempt to generate a file inside it, VFS will know that a folder is supposed to exist, and auto generate all the missing directories in overwrite needed for that new file, even though the file create operation was supposed to fail if parent folders were missing. This should all be invisible to the application.

So the takeaway for us here is that it seems there is a problem with std::weakly_canonical somehow returning the actual path, rather than the virtual one, and minimum code to reproduce this would be very helpful.

The takeaway for you would be that RedFileSystem is supposed to use the Virtual paths, not the actual MO2 paths. Using those virtual paths, we expect the VFS to give back the results you would expect as if it weren't there.

AnyOldName3 commented 3 weeks ago

So the unexpected behaviour here is the fact that std::weakly_canonical is returning the actual MO2 path, rather than the virtual path inside the game folder. This is breaking the virtualization.

This might be related to the USVFS bug that affects OpenMW, as that appeared when we switched from boost::filesystem to std::filesystem. We don't call weakly_canonical, but if the STL's internally canonicalising things sometimes and getting the real path, that would explain some of the error messages.

poirierlouis commented 3 weeks ago

@Al12rs thank you for all the details.

The expected behaviour of running a program or tool through the MO2 Virtual File System, is that it would not be able to notice it is being virtualized or not.

It was indeed a misconception on my part at first, to try and workaround issues met with MO2 VFS.

So the unexpected behaviour here is the fact that std::weakly_canonical is returning the actual MO2 path, rather than the virtual path inside the game folder. This is breaking the virtualization.

I understand that it is an issue from MO2 VFS then. And I should be able to use it regarding the security use case I talked about. Without trying to workaround. Which brings me to:

I think the issue is that RedFileSystem is trying to use the Actual path that you get returned by std::weakly_canonical (/mods/HUDPainter/r6/storages/HUDPainter/DEFAULT.json).

Currently, and it only applies regarding bug A and B, std::weakly_canonical is no longer used.

The VFS will not actually generate the empty folder in overwrite, but it should keep track of it in memory. So if you attempt to generate a file inside it, VFS will know that a folder is supposed to exist, and auto generate all the missing directories in overwrite needed for that new file, even though the file create operation was supposed to fail if parent folders were missing. This should all be invisible to the application.

I take it is an issue from MO2 VFS on this matter, and RedFileSystem is not supposed to use create_directories before writing/creating said file. Like you mentioned, it should be transparent.

The takeaway for you would be that RedFileSystem is supposed to use the Virtual paths, not the actual MO2 paths. Using those virtual paths, we expect the VFS to give back the results you would expect as if it weren't there.

Like stated above, this is all good for me.

Anyway, I'll come back to you with an example ASAP.

qudix commented 3 weeks ago

std::weakly_canonical eventually calls GetFinalPathNameByHandleW, though the ReactOS doc for it isn't implemented, so I don't know what it calls in turn. There were discussions about hooking this same api function in discord last year, so I assume this would be a good place to start.

AnyOldName3 commented 3 weeks ago

That definitely sounds like something we should be hooking seeing as we're giving a handle to the real file and unhooked, it'd give the non-VFS path.

poirierlouis commented 3 weeks ago

I actually found a mistake on my end when using directory_iterator, sorry for that. FYI, culprit of bug B is commented in example:

rfs_mo2_vfs_issue.txt const& not present for brevity

Two issues then:

Edit: fix attachment.

Al12rs commented 3 weeks ago
  • creating directory of storage. Currently use create_directories workaround, which should not be required.

@poirierlouis I'm not sure I got everything correctly.

My assumption is that all issues can be traced back to the fact that some std filesystem functions are apparently returning the actual path instead of the virtualized path, and that if you were to use the virtual path, it would work correctly.

Are you saying that even if you make sure the paths passed are all virtual, and you create the parent folder and then the child file, it still fails?

My assumption was that it failed because the path constructed for TEST.json ended up being Actual rather than virtual, so it would attempt to create the file directly in overwrite (<MO2>/overwrite/r6/storages/HUDPainter/TEST.json) instead of attempting in storages\HUDPainter\TEST.json.

Are you saying that is not the case and that there is a problem with file creation on top of std::weakly_canonical and potentially other functions somehow returning the wrong paths?

poirierlouis commented 3 weeks ago

Are you saying that even if you make sure the paths passed are all virtual, and you create the parent folder and then the child file, it still fails?

Yes. It will first test if path exists in <GAME>/r6/storages/HUDPainter, using request_directory. While it doesn't exist in <MO>/overwrite/r6/storages/HUDPainter, std::filesystem::exists returns true, because of <MO>/mods/HUDPainter/r6/storages/HUDPainter. In such case, it doesn't call create_directory to create path <GAME>/r6/storages/HUDPainter (virtual path <MO2>/overwrite/r6/storages/HUDPainter).

Workaround is to actually call create_directories when reading / writing file. Such as in this case, path <MO2>/overwrite/r6/storages/HUDPainter is created. And then IO operations on file are working as expected. Step with request_directory is meant to create storage directory once. But currently, it is mixed up somehow, workaround requires create_directories.

Are you saying that is not the case and that there is a problem with file creation on top of std::weakly_canonical and potentially other functions somehow returning the wrong paths?

I don't know, maybe, I lack knowledge about how MO2 VFS works under the hood to answer. I think that mapping between <MO2>/overwrite and <MO2>/mods is in the way. I would have thought overwrite contains everything, but I guess mods is here to add an extra layer of isolation. I'm wondering if this extra layer is the culprit somehow (like said above).

Al12rs commented 3 weeks ago

It will first test if path exists in <GAME>/r6/storages/HUDPainter, using request_directory. While it doesn't exist in <MO>/overwrite/r6/storages/HUDPainter, std::filesystem::exists returns true, because of <MO>/mods/HUDPainter/r6/storages/HUDPainter. In such case, it doesn't call create_directory to create path <GAME>/r6/storages/HUDPainter (virtual path <MO2>/overwrite/r6/storages/HUDPainter).

Workaround is to actually call create_directories when reading / writing file. Such as in this case, path <MO2>/overwrite/r6/storages/HUDPainter is created. And then IO operations on file are working as expected. Step with request_directory is meant to create storage directory once. But currently, it is mixed up somehow, workaround requires create_directories.

To explain the role of Overwrite, basically if a new file is created (not replacing an existing file) inside the Game folder, we wouldn't know in which mod folder to put this file, so we put it in a special mod we call Overwrite (because it comes last and potentially overwrites all other mods in case of conflicts).

So here is how the VFS is supposed to behave:

I know we have code to handle this in USVFS, so perhaps there is a bug somewhere, or at some point the paths get converted to actual paths, which then bypass the vfs, and that is the actual issue.

rfuzzo commented 3 weeks ago

So, the issue I have with that is:

Does it matter how the file is created? As in: should the path be absolute and resolve to overwrite, or to the game folder? Relative doesn't work as that will create the file in /bin

Holt59 commented 3 weeks ago

So, the issue I have with that is:

* creating a file in overwrite at runtime and then using directory_iterator doesn't show the newly created file in overwrite for some reason.

Does it matter how the file is created? As in: should the path be absolute and resolve to overwrite, or to the game folder? Relative doesn't work as that will create the file in /bin

It does not matter from a game point-of-view (or any hooked program), you should never have to deal with overwrite or mods folder.

From your point-of-view, it's either relative to the game folder or absolute (but absolute in the game folder, not in MO2 folder).

rfuzzo commented 3 weeks ago

Hmm thanks. I'll try with the full game path then. relative doesn't work as the file gets created in the script extender assembly directory (unless I do something like ../../../r6/storage/newFile.json I suppose

poirierlouis commented 3 weeks ago

I know we have code to handle this in USVFS, so perhaps there is a bug somewhere, or at some point the paths get converted to actual paths, which then bypass the vfs, and that is the actual issue.

I tried again this time without using std::weakly_canonical at all (assuming path is safe for this test). It is working fine. Virtual path (game folder) is used everywhere, no more problem to create directory, read/write file nor to list files.

In the end, std::weakly_canonical is the only culprit for now imo. It returns actual path (MO2 folder) instead of virtual path (game folder).

Al12rs commented 3 weeks ago

Perfect, thank you very much for testing this and reporting back! It is good to know the exact scope of the issue

The problem on the USVFS side then is that std::weakly_canonical is returning the actual path.

@qudix was saying that he managed to trace back the implementation of it to GetFinalPathNameByHandleW. I looked at the Wine implementation of it here: https://github.com/wine-mirror/wine/blob/951e0e27a743e52c75c7fedc0b1eaa9eb77e6bb6/dlls/kernelbase/file.c#L1875

I think we should be able to add a hook to GetFinalPathNameByHandleW that simply calls the original function, and then checks if the returned path is redirected, if it is, we replace it with the virtual version and return that back to the caller.

Sewer56 commented 2 weeks ago

When I was chatting with @Al12rs earlier today after some work stuff, he mentioned this issue. Funnily enough, I immediately knew which API was the culprit from the description.

Anyway, I've had a double check with some Windows binaries, in addition to Wine

You always want to go for the lowest level user mode functions. So shoot straight for ntdll. I checked the relationships with the higher level APIs of course.

Anyway first thing you need to hook is NtQueryObject, and handle the case when the OBJECT_INFORMATION_CLASS parameter is set to ObjectNameInformation (1). Return the full path from the API, that alone should satisfy Wine.

However, this is not the only function needed. For Windows, you also need to hook NtQueryInformationFile if you're not doing that already, as that contains FileNameInformation (9), FileAllInformation (18), FileNormalizedNameInformation (48) as possible parameters. Do return the full path in these APIs.

For FileNormalizedNameInformation, I don't believe it's very well documented anywhere, but you don't need to do any special casing for it. Basically each 8.3 directory name is expanded, and the casing of the path should match the one in the FileSystem. If you feed the exact full path of the file that's being redirected (provided you got it from the FS), as the response you should be okay. Oh and obviously use backslashes.

Note: In some old snippets of reversed source code on the web, you'll also find ObjectAllInformation (3) for NtQueryObject That name is misleading, so don't pay mind to it. Nothing changed there, the updated name is more accurate.

I've also written a VFS before, from scratch before I knew USVFS existed (I kinda found out it's been done before 70% into development). Mine was a read only one though 😉

Hope that helps. Happy Hacking :+1:

Al12rs commented 2 weeks ago

Thanks @Sewer56 🙏