OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Defining ResourceManager behaviour #232

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

I'm having trouble reasoning through how we want ResourceManager to behave. For example, lets say that you construct ResourceManager by passing it "C:/Test". ResourceManager will search C:/Test for archives and register them.

Now you want to load file well0001.bmp. This file is contained in C:/Test loosely (not in volume). What do you pass into GetResourceStream()?

I think the answer is option 1. You just pass filenames into ResourceStream. If you know the directory the file is contained in, then you don't use ResourceStream.

I didn't spend a lot of time properly defining what ResourceStream's behavior should be up front. I guess we are paying for that now.

Thanks, Brett

DanRStevens commented 5 years ago

I agree with option 1. This seems to match how Outpost 2 works, and other VOL file processing code, such as the OP2MapEditor.

The ResourceManager class implies a search path. Currently it supports a single filesystem folder, and multiple archives as part of the search path. It finds the first match by (effectively) pre-pending the given relative path with each of the search locations (though there is no actual string concatenation in the case of archives).

For both simplicity and security reasons, GetResourceStream should only ever be used with relative paths. It should also probably disallow .. as path of that relative path.

Brett208 commented 5 years ago

Sorry for being dense, but what is the ResourceStream class? I don't remember it.

I was planning on disallowing all paths, relative or absolute to simplify. This would solve the security concerns without having to sort what type of directory is provided. Is it important to you that ResourceManager allows searching relative paths, minus ..? If so, I can spend some cycles working that out.

I was only planning on supporting a single location with ResourceManager. It sounds like you are suggesting we open it to allow adding multiple directories? If you are wanting it, I'll spend some time working out the details.

Thanks, Brett

DanRStevens commented 5 years ago

Err, that was a typo. I meant ResourceManager. The ResourceManager object is basically a search path implementation, that supports loading archives into its own virtual filesystem.

The path restriction is more of a nice to have. I'm fine with punting on that and just allowing any path for now. We're not using the class in any context that is particularly security sensitive. If we got to a point where players were exchanging files such as new maps, and using this code, then yes, I'd want to have path restrictions in place for that.

I'd like to keep things simple with a single root filesystem folder as part of the search path. Ideally all subfolders of interest will be subfolders of this root anyway, so it would be fairly natural to allow relative paths to descend deeper into the folder tree. That might be useful for a mod installed in a subfolder to access its own files.