Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
poirierlouis opened this issue May 30, 2024 · 20 comments · May be fixed by ModOrganizer2/usvfs#68
Open

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

poirierlouis opened this issue May 30, 2024 · 20 comments · May be fixed by ModOrganizer2/usvfs#68
Labels
issue report User submitted report

Comments

@poirierlouis
Copy link

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:

  • Mod Organizer 2.5.0
  • Windows 10.0.19045.4412

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):

  • <GAME>/r6/storages/HUDPainter
  • <MO2>/mods/HUD Painter/r6/storages/HUDPainter
  • <MO2>/overwrite/r6/storages/HUDPainter

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.

@poirierlouis poirierlouis added the issue report User submitted report label May 30, 2024
@Holt59
Copy link
Member

Holt59 commented May 30, 2024

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
Copy link
Author

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
Copy link
Member

Holt59 commented Jun 1, 2024

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
Copy link
Author

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
Copy link
Member

Al12rs commented Jun 1, 2024

@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
Copy link
Member

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
Copy link
Author

@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
Copy link
Contributor

qudix commented Jun 1, 2024

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
Copy link
Member

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
Copy link
Author

poirierlouis commented Jun 1, 2024

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:

  • std::weakly_canonical
  • creating directory of storage. Currently use create_directories workaround, which should not be required.

Edit: fix attachment.

@Al12rs
Copy link
Member

Al12rs commented Jun 1, 2024

  • 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
Copy link
Author

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
Copy link
Member

Al12rs commented Jun 1, 2024

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:

  • Test if <GAME>/r6/storages/HUDPainter exists should return true, because it does exist in <MO>/mods/HUDPainter/r6/storages/HUDPainter. Overwrite is supposed to remain empty at this stage, because you are not creating anything new.
  • Attempting to create the new <GAME>/r6/storages/HUDPainter/TEST.json file should succeed without any PathDoesNotExist errors, as VFS should recognize that <MO>/mods/HUDPainter/r6/storages/HUDPainter already exists inside a mod, so it will create the missing directories inside Ovewrite and should create the file in there and return success.

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
Copy link

rfuzzo commented Jun 2, 2024

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

@Holt59
Copy link
Member

Holt59 commented Jun 2, 2024

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
Copy link

rfuzzo commented Jun 2, 2024

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
Copy link
Author

poirierlouis commented Jun 2, 2024

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
Copy link
Member

Al12rs commented Jun 2, 2024

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
Copy link

Sewer56 commented Jun 6, 2024

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

  • Win 11 23H2
  • Win 10 20.04
  • Win 7

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 👍

@Al12rs
Copy link
Member

Al12rs commented Jun 6, 2024

Thanks @Sewer56 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue report User submitted report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants