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

Move IO-related operations to a helper .dll #164

Merged
merged 25 commits into from
Oct 15, 2022
Merged

Conversation

kartoFlane
Copy link
Member

@kartoFlane kartoFlane commented Sep 10, 2022

In some places in the code base we need to access the file system, for example to read a file, check if a file exists, or walk a directory. Lua is not a particularly good fit for this, and in some cases where we need speed, we ended up resorting to running native system commands via os.execute.

This has some issues, most notably security concerns that - thankfully - we have thus far avoided. Another is that when running native system commands, a command terminal windows briefly pops up, which is not only distracting, but might cause some people to think something went wrong.

This PR addresses the latter concern, and attempts to partially address the former, via a helper itb_io.dll (source code: https://github.com/kartoFlane/itb-io-rs). This library provides File and Directory abstractions, which expose several helper functions for file system navigation, reading, and writing. I've replaced almost all instances where we needed to access the file system in the mod loader with these functions. If we need additional functionality in the future, it should be simple to add.

For security, the library throws an error whenever Lua code attempts to navigate away from the game's root directory, the game's save data directory, or any of their children. This security measure is based on abstract paths that are not validated against the filesystem unless strictly necessary, so it is possible to "break out" in a limited fashion, if a directory happens to be a symlink, but this issue should have a limited impact, as realistically it only affects power users. The library gives Lua full reign over anything inside of those directories.

Basically:

-- works:
Directory("path/to/Into the Breach/resources")
Directory("./resources")
Directory("resources")

-- errors:
Directory("path/to/Into the Breach/..")
Directory("path/to")
Directory("path/to/Into the Breach"):parent()

Available methods:

  • File
    • constructor: File new(string) - creates a new File instance
      local f = File("resources/resource.dat")
    • string path() - returns the File's full path
    • string name() - returns the file's name, including extension
    • string name_without_extension() - returns the file's name, without extension
    • string extension() - returns the file's extension, or nil if it doesn't have any
    • Directory parent() - returns a Directory instance of the file's parent directory
    • string read_to_string() - contents of the file as string
    • table read_to_byte_array() - contents of the file as byte array
    • void write_string(string) - replaces the file's content with the specified string content
    • void append(string) - appends the specified string at the end of the file
    • void write_byte_array(table) - replaces the file's content with the specified byte array content
    • void copy(string) - copies this file to the specified destination path, overwrites anything at destination. Returns a new File instance for the destination path
    • void move(string) - moves this file to the specified destination. Returns a new File instance for the destination path
    • void make_directories() - creates missing directories in this File's abstract path
    • boolean exists() - returns true if the file exists, false otherwise
    • void delete() - deletes the file
  • Directory
    • constructor: Directory new(string) - creates a new Directory instance
      local d = Directory("resources")
    • static Directory savedata() - returns a Directory instance for the save data directory
    • string path() - returns the Directory's full path
    • string name() - returns the Directory's name
    • Directory parent() - returns a Directory instance of the directory's parent directory
    • File file(paths...) - returns a File instance for the specified path under this directory
    • Directory directory(paths...) - returns a Directory instance for the specified path under this directory
    • table files() - returns a list table of Files within this directory
    • table directories() - returns a list table of Directories within this directory
    • void make_directories() - creates missing directories in this Directory's abstract path
    • boolean exists() - returns true if the directory exists, false otherwise
    • void delete() - deletes the directory and all its contents

@KnightMiner KnightMiner added enhancement New feature or request cleanup Issues and pull requests related to improving the codebase without direct functional changes labels Sep 18, 2022
@Lemonymous
Copy link
Contributor

Lemonymous commented Sep 21, 2022

The following (3) hard crashes the game instead of throwing an error:

  • Calling directories on a non-existing directory leads to hard crash
dir = Directory("randomGibberish") 
dir:directories() 
  • Calling files on a non-existing directory leads to hard crash
dir = Directory("randomGibberish") 
dir:files() 
  • Moving outside of the allowed directories leads to hard crash
dir = Directory("randomGibberish") 
dir:parent():parent() 

directories returns a list table of directories, but using save_table on it throws the error: attempt to get an unknown field 'GetLuaString' (same for files)

dir = Directory("mods") 
LOG(save_table(dir:directories())) 

Moving a file does not update the File instance to the new path. Not sure if intentional or not.

file = File(GetSavedataLocation().."blah.txt") 
file:write_string("blah") 
file:move(GetSavedataLocation().."blahMoved.txt") 
LOG(file:path()) -- returns "blah.txt" 

@kartoFlane
Copy link
Member Author

kartoFlane commented Sep 22, 2022

Fixed the hard crashes and tweaked the move and copy methods to return a new File instance for the destination path, if they complete successfully, so you can now do:

file = File("blah.txt")
file:write_string("blah")
file2 = file:move("blah_moved.txt")
LOG(file:path()) -- old path
LOG(file2:path()) -- new path

 

For the save_table issue, I've added GetLuaString functions to both File and Directory (so you can just write LOG(file) instead of LOG(file:path()) now), however save_table didn't honor this function like LOG does, so I needed to change it.

This might have unintended consequences if some code relies on this behavior, but I think it's the correct way of handling this case, since as far as I'm aware, save_table is supposed to serialize the table to a string format in such a way that it can later be deserialized, ie. re-constructed by evaluating the string in Lua, eg. via

tbl = { 1, 2, 3 }
serialized = save_table(tbl)
result = loadstring(serialized)()
-- Edit: maybe not quite loadstring, but the game uses something like this to load save data files,
-- which are serialzied lua tables

without this change, Directories and Files would be saved as regular tables with a single instance field, so it wouldn't be possible to recreate them in this fashion.

@Lemonymous
Copy link
Contributor

Lemonymous commented Sep 22, 2022

All my previous points seems to have been addressed, and works well now.
Tried rewriting some of the os commands for Easy Edit, and a few short tests went well.

While testing it with Easy Edit, I came over a few other minor "issues", or maybe just nitpicks.

  1. I am not sure if this is within the scope of what you want this to be, but I feel os.mkdir might be missing.

  2. Should not directory paths end in / so one can concatenate in the following manner?

  • Throws error because it concatenates it as ..documents/my games/into the breachblah.txt
File(Directory.savedata():path().."blah.txt")
  • This works
File(Directory.savedata():path().."/blah.txt")
  1. Accessing files and directories within the savedata folder seems overly verbose
Directory(Directory.savedata():path().."/profile_alpha")

Could the static function savedata have an optional argument that concatenates with the savedata path?
i.e.

Directory.savedata("profile_alpha")

Similarly, accessing files in savedata is equally verbose. Not sure if you would agree to add a static savedata function in File as well which could be used as follows. (The path argument would not be optional in this case, since just File.savedata() wouldn't mean anything)

File.savedata("profile_alpha/modcontent.lua")

@kartoFlane
Copy link
Member Author

I am not sure if this is within the scope of what you want this to be, but I feel os.mkdir might be missing.

mkdir calls are included when trying to create a file (ie. via write_x or move/copy functions), but I can add that onto Directory instances as well.

Should not directory paths end in / so one can concatenate in the following manner?

Directory class already includes file and directory functions which take care of adding the trailing slash if it's needed, you can use them like so:

local myFile = Directory.savedata():file("blah.txt")
local myProfile = Directory.savedata():directory("my_profile")

I think I'd rather keep paths without trailing slashes to discourage manual concatenation of strings.

@Lemonymous
Copy link
Contributor

Lemonymous commented Sep 23, 2022

I am not sure if this is within the scope of what you want this to be, but I feel os.mkdir might be missing.

mkdir calls are included when trying to create a file (ie. via write_x or move/copy functions), but I can add that onto Directory instances as well.

I wasn't aware that writing a file would create directories for the full path for you actually, so maybe it wasn't strictly needed. I still think the ability to create empty folders can be useful though, so thanks for this addition.

Should not directory paths end in / so one can concatenate in the following manner?

Directory class already includes file and directory functions which take care of adding the trailing slash if it's needed, you can use them like so:

local myFile = Directory.savedata():file("blah.txt")
local myProfile = Directory.savedata():directory("my_profile")

I think I'd rather keep paths without trailing slashes to discourage manual concatenation of strings.

I must have completely missed those two methods: file and directory.

From your examples, I suppose getting a file under savedata will look something like this:

local myFile = Directory.savedata():file("my_profile/modcontent.lua")

I do find this to look rather neat.

There is no use of path in this string of function calls though, and I am not sure why you would call path for a directory if you don't intend to concatenate something to it. Wouldn't having the trailing slash be more correct for a path? In addition to being a working path that can be concatenated to, it also very clearly indicates that it is a directory path, and not a file path.

I will test how using those functions will change things in practice.

@Lemonymous
Copy link
Contributor

I've now rewritten Easy Edit, exclusively using functions from this PR for io operation. Everything seems to be working well. I still believe output from Directory.path should have a trailing slash, but that may just be my preference.

One thing that came up was how some of the mod loader's functions take arguments that are paths relative to the save game. sdlext.config being one such function. Having to work with relative paths, did lead to some need for concatenation. Not sure if anything can be done about it though.

I am finished testing this branch, and think it can be merged.

Also rework it to use a buffer instead of writing directly to the file
- File and Directory report paths using slashes instead of backslashes
- Directory reports its path with a trailing slash
@kartoFlane
Copy link
Member Author

I ended up adding the trailing slash for directories - I still feel kinda iffy about it, since there isn't really any standard like that, and most languages/frameworks I'm familiar with don't do that, but I guess it makes sense in the context of Lua, and is convenient. It doesn't seem to break anything on the Rust side, just gets pruned.

Also fixed path() function to report the path using slashes instead of backslashes, so it's more readable in the console, since backslashes bug out and get printed as #. Windows technically uses backslashes, but accepts slashes anyway, so it doesn't really matter.

I also changed sdlext.config to use the new API, and optimized it a little so that it constructs the string in a buffer, rather than making a lot of small piecemeal writes directly to the file, which is very slow. This will conflict with #171, but is simple to fix (just change file:write(x) to table.insert(buffer, x))

@Lemonymous
Copy link
Contributor

Lemonymous commented Oct 7, 2022

Instances where os is used in the mod loader instead of itb_io.dll

modApi/ftldat.lua:96 - os.listdirs
modApi/ftldat.lua:99 - os.listfiles
modApi/map.lua:12 - os.listfiles
modApi/map.lua:24 - os.remove
modApi/savedata.lua:24 - os.getKnownFolder
mod_loader.lua:315 - os.listfiles
mod_loader.lua:316 - os.listfiles
mod_loader.lua:333 - os.listdirs
mod_loader.lua:334 - os.listdirs

Instances where io is used in the mod loader instead of itb_io.dll

logger_basic.lua:86 - io.open
logger_basic.lua:91 - io.open
modApi/statistics.lua:14 - io.popen
mod_loader.lua:319 - io.popen
mod_loader.lua:337 - io.popen

@kartoFlane
Copy link
Member Author

Removed all mentioned usages of os and io, added File:append to append strings to a file

Copy link
Contributor

@Lemonymous Lemonymous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6980810


for _, dir in ipairs(dirs) do
dir = dir:name()
local modDirPath = inputDir:name().."/"..dir.."/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When enumerating submods, inputDir:name() is incorrectly just mods, which makes loading of submods fail.

@Lemonymous Lemonymous merged commit 178cd95 into master Oct 15, 2022
@Lemonymous Lemonymous deleted the feature/itb-io branch October 15, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Issues and pull requests related to improving the codebase without direct functional changes enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants