Hi,
I’ve been working on a project to update the file handling in ioquake3. It’s mainly intended to perform better and be more stable in configurations with a large number of pk3s installed. It also has some security fixes and other improvements.
Here are the links:
https://github.com/Chomenor/ioq3_new_filesystem
https://github.com/Chomenor/ioq3_new_filesystem/blob/master/new-filesystem-readme.md
I don’t know if it will be useful to you guys, but I’d be happy to hear what you think of it.
Here are some Windows builds if you want to give it a try:
Hi,
Okay, I tested this project, and I have to say it runs fine!
Although I’m definitely not an expert with the details of this project, the project seems very promising to me! Eventually someone with more knowledge should check it out!
@Chomenor what are exctly the ’security improvements’ you’re talking about in the readme.md?
Hi ToKu,
Thanks for testing, I’m glad it’s working for you!
The security improvements are a good question. I would start with path sanitization - in my filesystem almost all read and write operations go through the fs_generate_path function, so there is a central place to check for invalid or malicious paths. In the existing code there is a lot less consistency in how paths are created and validated, and I did find a couple of specific issues that could potentially be exploited by a malicious running VM. I don’t know if the ioq3 devs would prefer if I post the details here or send it to them privately so they could issue a patch first.
I would also say that the new filesystem is more secure in terms of how hard it is for a bad pk3 to mess up the game. Since the ID paks have automatic precedence a server can’t just download a pk3 to baseq3 that permanently overrides the VMs and other core files. There are still ways a pk3 can mess up the game, but they are more limited in practice than they were previously.
There are some download-related improvements as well, including the fs_saveto_dlfolder and fs_restrict_dlfolder options, and also validating the hash of files after they are downloaded to make sure they don’t match an existing file. That’s to help prevent a server from slipping in a spoofed version of a system pak or other important file.
I added a cvar called fs_search_inactive_mods, with the following functionality:
This should help address the feedback from @SmileTheory in the security thread. Note that these features could be set to a different default or hard-coded should the project be merged with ioquake3.
I also realized there is a way to support the old shader precedence in the new flesystem. I could add a cvar to the renderer, such as r_use_new_filesystem, that would let you switch off all the new filesystem calls and revert to the old shader scanning code, as if you had used an old-filesystem renderer dll. This should be straightforward to implement if anybody is interested.
Yeah, already updated my test code with your latest changes!
Quick question not related to the latest changes:
Does this one: https://github.com/Chomenor/ioq3_new_filesystem/blob/master/code/client/cl_cgame.c#L754 (inside the #ifndef NEW_FILESYSTEM now) mean that with the new file system also DLLs can be loaded? I think only QVMs should be allowed to load on sv_pure? Or do I understand this wrong? I’m no expert!
Well, I’ll contact you soon, because there are more questions… Don’t be discouraged! Testing still continues…
Dll files shouldn’t be returned by the lookup on pure servers anyway, since dll files can’t come from a pk3, and only files from pk3s get loaded when connected to a pure server. Specifically, only pk3 sourcetype files get a server_pak_position value (fs_lookup.c#L57), and files with no server_pak_position are discarded when connected to a pure server (fs_lookup.c#L92).
Also cl_connectedToPureServer doesn’t exist anymore, since I figured it was no longer necessary to store that state in the client as well as the filesystem. I try to avoid having the same state stored in multiple places as much as possible. That was probably the original reason for the ifndef, even though it does give the wrong impression from looking at it.
Thanks again for the testing and questions.
Ah, okay! I’m simply a bit wary concerning DLLs. Thanks for the prompt answer!
I see filehandle ownership - which is good. But I don’t see leaked filehandle cleanup on VM shutdown.
Also, it is much better to see some definitions instead of hardcoded constants like 1 2 3 etc.
Hi ech,
I didn’t consider the leaked handle cleanup a high priority since it wasn’t a security issue, and there are many other ways a buggy mod can cause the game to crash. However since you’re interested I’ll look into adding it, as well as changing the constants.
I’m curious as to the specific vulnerability this change is meant to address, since the fix doesn’t seem very robust. First, CVAR_PROTECTED can currently be bypassed by a VM using console command traps and a set command. Second, if there is an attack to trigger execution of dlls with a non-dll extension, blocking .pk3 extension specifically is likely not sufficient, since a pk3 can contain a VM and a VM can write binary files of arbitrary extensions to the same location as the pk3. Doing a sanity check against all non-dll extensions in Sys_LoadDll would seem more prudent than only checking against the pk3 extension.
EDIT: For general protection against sprintf overflow issues and other things, I think it would be good to have an explicit check for the correct extensions directly ahead of all calls to Sys_LoadLibrary.
Also note that these changes do not fix the vulnerabilities I referred to in my filesystem project post. If you would like me to report those vulnerabilities, let me know where I should send the report (i.e. to a specific person, or publicly on the forum or github).
Hi, I just want to say that I wonder why such projects from Chomenor aren’t part of ioquake3? I am still testing Chomenor’s project and it runs fine. @MrNuclearMonster, wouldn’t Chomenoer’s project help ioquake3 to become more secure? Eventually before releasing a new build?
@Chomenor, still playing around with your project, it runs fine, but I already said this to you…, exellent work!
Usually because they don’t suit the scope of ioquake3.
If you don’t mind me asking, what is the scope of ioquake3 that you are referring to? The filesystem project has load time improvement, stability, security, bugfixes, etc. but it really doesn’t change the gameplay or break compatibility with maps, mods, or servers. There are some new features but almost everything is cvar controlled. I would think it would be in a similar category as included projects like voip, cURL downloads, or the gl2 renderer in terms of relevance to ioquake3.
I do understand the filesystem is a bigger change in terms of the core ioq3 code, but there are a lot of issues and limitation with the code it is replacing. I don’t think a change should be rejected just because it is large if it is a significant improvement over the existing code.
EDIT: I’d also like to mention that the new filesystem isn’t as complicated as it might look at first glance. Once you understand the basic architecture I think it’s actually easier to work with than the old filesystem, and has some nifty features as well. I could post an architectural overview if you’d like.
The main difference here is testing and age. The OpenGL2 renderer was developed for two years before it was accepted into ioq3 proper, and tested and improved for another four years before it became default renderer. And believe you me, it still has bugs I don’t have fixes for.
Completely removing and renovating the filesystem is a much bigger deal than this. Rushing in a patch like this could introduce subtle bugs that don’t crop up until much, much later.
But let’s say your code is perfect, and you’re a much better programmer than I am. Ultimately, someone has to maintain it. The OpenGL2 renderer is part of ioquake3 because I maintain it. Who will maintain this filesystem, and also, who will maintain the old one?
Also, glancing over your fork, if I’m reading it correctly, you’ve basically taken the functionality of /code/qcommon/files.c and split it up over ten files or so in /code/filesystem/, and then ripped out bits of file management from across the codebase and slipped it into /code/filesystem/fscore/. People complain that my patches change every little file, but you’ve basically unleashed a spider here. 
The original code was written to work with a dumb pipe file system, but your changes introduce extra coupling, tying the renderer to the filesystem for example. Also, correct me if I’m wrong, but I think I saw at least two implementations of a hash table in there.
Sorry if I’m coming off as a little ranty. I don’t mean to discourage contribution, and I’m sure the OpenGL2 renderer changes have done some of what you’ve done and probably worse. I just don’t want to be faced with maintaining an increasingly alien codebase.
Thanks for the feedback. I’ll try to address your concerns:
I don’t advocate that my filesystem would be rushed out without testing. I think a couple of years would not be unreasonable. There is also the option to integrate it with the ifdefs in place as a temporary stage. The specifics of the review, testing, and consideration for release process would be up to you and the other maintainers. For my part I expect to keep my project fork synced with ioq3 along with other tweaks and updates for at least a few years, so there isn’t really a rush.
Of course no code is perfect, but the standard to improve on the existing filesystem isn’t that high. People have had to take for granted that adding a lot of paks will make the game slow and unstable, and that a single bad pak can easily break the game. My filesystem can fix 99% of those problems. There are multiple tricky issues with the download support where downloads fail or go into a loop due to poorly configurated servers, and in practical usage my filesystem succeeds in connecting to a lot of servers where the old client fails. My filesystem fixes a whole host of outright and potential security, crash, and other issues.
I’m not a better programmer but I do have experience with this particular area of the code. I would help maintain if the project was integrated, since obviously I want people to have the best experience with it as possible. The new filesystem should be able to support everything the old filesystem does without needing to maintain both versions long-term.
The functionality of files.c is implemented in /code/filesystem/. The fscore is actually a separate component that can be compiled and used separately from the game. It handles the core file indexing functionality and you can think of it as sort of a library the rest of the filesystem uses, although it could be useful for other things like mapping tools as well. All the fscore files are written from scratch except fsc_md4.c and fsc_gameparse.c, which are needed for pk3 hash calculation and shader parsing, respectively.
The file structure is my best effort to separate the code into coherent parts. I personally think it is easier to work with than the old files.c, but it’s hard for me to know how other people will like it.
The renderer already had some filesystem calls, but I needed to add new ones to make functional improvements. The shader selection is moved into the filesystem so shaders can be prioritized with the full range of data available to the filesystem, and so shaders and default shader images can be prioritized side by side. The filesystem only parses the name and basic structure of shaders, not what’s within the brackets, so it doesn’t affect functionality of any normally formatted shaders.
It’s important to note the renderer can function without the extra functions and you can currently run an old renderer dll with the new filesystem without significant issues.
The two hash tables use memory differently. Not perfectly elegant solution but it works 
No problem. As I said, I think I can help maintain the filesystem, so that could be a positive thing. I like the new renderer, and I think it’s mutually beneficial with the filesystem. There are thousands of really good fan-made maps for quake 3, and better file handling and better graphics combined make for a very good experience.
I’d really like to avoid this. The only thing ifdefs will do will ensure one path never gets tested. Using the OpenGL2 renderer as an example, it has a lot of code duplicated from the original OpenGL1 renderer, but compiling out both at least helps both get tested.
My main problem is that there are certain design decisions in your filesystem that I don’t agree with. Right now, I feel the filesystem should be simplified and with a smaller API, unlike yours which is growing and has new entry points to access new functionality. I’d prefer if the filesystem didn’t make any changes or assumptions about what the rest of the code wants.
Features-wise, looking through your readme, I disagree with a number of features. Searching for resources across all mods is a definite misfeature. If I’m developing a map, I’d like missing textures to show up as missing, especially if I want vanilla Q3 support. If it happens to be grabbing a texture from whatever mod I happened to have installed and displays it as normal, I’d have no clue that there was a problem. And even if there’s a cvar to deactivate this functionality, that’s another thing that needs to be documented and moreover noticed. Sometimes everything magically working isn’t the right solution.
Another feature I disagree with is the shader precedence. This has been discussed before (https://bugzilla.icculus.org/show_bug.cgi?id=4021) and rejected back then. I tend towards accepting previous judgements unless there’s a really, really convincing reason not to. I might be willing to accept it if it defaulted to off and enabled by cvar, but I don’t want to deal with any headaches involving people turning it on and asking why their textures are suddenly different.
Really, the problem is that this patch covers far too much functionality to integrate right now. It’d be nice if it was several smaller patches separating fixes from improvements from (mis)features.
Also, this is extremely personal, but I hate how you use }. “statement(); }”, really?
And sorry if this is ranty again, I’m really not used to responding to people on forums. I used to be the guy that just quietly improved his corner of the code, and then that turned into the whole codebase, and now I have to actually talk to people.
Ifdefs do allow you to compile out both versions, but what gets tested depends on the binaries that are distributed. It only mentioned ifdefs as one option, I know it has downsides.
On a quick count, original filesystem has 58 shared function vs 74 for my project. Most of the increase is directly related to new features or code quality improvements. If you would like to point me to some specific added functions I’ll try to explain the design decisions.
For any given functionality, there’s often a tradeoff between exposing more internal state of the filesystem or moving more specialized functions to the filesystem. My general rule is I try to have everything in the filesystem be at least mostly primarily file handling related. For example I rolled more download handling stuff into the filesystem, but it’s all related to selecting which files to download, how to save them, etc. This stuff naturally fits in the filesystem and is easier to maintain there and also port to other projects. The actual network download stuff remains outside the filesystem.
I’m not sure it’s so black and white when it comes to practical situations that arise on online servers. For example maps are often downloaded to mod directories, and if you go to another server running the same map with a different mod it helps to be able to access that map. That said, this is a relatively easy thing to change, so it can be adjusted however you want if you choose to integrate with ioq3. I will look into adding a cvar for this.
That discussion was about naively reversing the precedence order under the old filesystem, which isn’t feasible. The new filesystem is a much more sophisticated system which prioritizes the ID paks over the current map pak over other paks, which are then handled by filesystem precedence. The new rules produce far better results in testing with a diverse load of paks installed, are easier to understand, and easier to debug with the new comparison functions. I would be very interested if you can find a bug under any released map or mod caused by the new shader rules. It’s not easy.
This isn’t trivial to implement, and I wouldn’t recommend it anyway, it would just add complexity. Problems with the new shader rules are very rare (unlike problems with the old rules which are very common) and there are better workarounds if they do come up.
Practically speaking, you could release the new filesystem but with some of the features disabled by default. It would narrow the search window a bit for new bugs discovered in the early release phase, and those settings could be re-enabled later.
Again, I’m not trying to rush anybody into integrating the project right now, but I don’t think it is necessary to throw it out completely at this stage, either.
Yeah, my personal coding style is weird, I admit it. I’m sure there’s automatic ways you could change it though.
I’ve updated the download folder handling to allow qvms from a trusted hash list when fs_restrict_dlfolder is set to 1. This should provide pretty much the same security as blocking qvms entirely, but with far better compatibility (over 95% compatibility with current download-enabled servers versus <40% with all qvms blocked). The old behavior of blocking all qvms is still available with fs_restrict_dlfolder 2.
The allowed qvms are currently stored as a hardcoded list of sha256 hashes here. The mods were selected from ones currently in use on download-enabled servers. More can be added later, of course. This is NOT a comprehensive list of all mods supported by ioq3, it is just meant to allow the most popular mods to work under strict security settings that would otherwise block all qvms. It also does not influence the ordering of paks, except for ones that fail the hash test; that is something to consider for a later feature.
Currently, the error handling approach is to throw an ERR_DROP on hash miss when connected to a pure server, and fall back to the best available qvm when not on a pure server. This helps provide an informative error message in the pure server scenario (where a specific qvm is likely required) but also avoids the fragility of a single nonessential pk3 causing a mod to permanently error out on non-pure servers.
If you want to test it, you can get a test build from here. For connecting to servers, you will probably want to set fs_saveto_dlfolder to 1 and fs_restrict_dlfolder to 1, as documented here. You can also run the “find_vm cgame”, “find_vm ui”, or “find_vm qagame” commands to show a list of all the qvms you have installed, their hashes, and their trusted status. Let me know if you have any feedback or want something added to the hash list.
I added a new documentation file that should be helpful for anybody who wants to know more about the design/implementation of this project: