minetest icon indicating copy to clipboard operation
minetest copied to clipboard

Add `minetest.file_exists`

Open Wuzzy2 opened this issue 1 year ago • 10 comments

Problem

There is currently not an easy way in Lua to check if a file exists. If the user wants to import a schematic in schemedit by typing in the name, the mod currently has to assume the file exists.

Solutions

Add minetest.file_exists(path). path is a full absolute path to the file. If it exists, returns true, false otherwise.

Alternatives

N/A

Additional context

No response

Wuzzy2 avatar Jul 27 '24 08:07 Wuzzy2

There is currently not an easy way in Lua to check if a file exists.

local function file_exists(path)
    local f = io.open(path, "r")
    if f then
        f:close()
        return true
    end
    return false
end

sfan5 avatar Jul 27 '24 10:07 sfan5

It would be nice to have this as a util as I have to include this in most mods I make. It's common in standard libraries

rubenwardy avatar Jul 27 '24 11:07 rubenwardy

I am concerned that this will lead modders to write worse code in 99% of cases. I think at the very least a note would be appropriate which tells modders to use this if and only if they want to check only whether a file exists, and not open it.

Whenever you want to open a file, you should always use local f, err = io.open(filename, mode) instead.

The fact that a file exists does not mean that you will subsequently be able to open it with the desired mode. The file could be visible to you, but you might lack the permission to write to it, for example. Or (arguably a theoretical issue) there could be a race condition where the file appears or disappears in between your existence check and your opening of the file.

In other words, if you're writing code like (I definitely did do this in the past):

local function do_something(filename)
    if not file_exists(filename) then return nil, "file doesn't exist" end
    local f = io.open(filename)
    ...
end

you're doing it wrong. You still need to check f and report an error if you failed to open it. At which you point you might as well write this the right way to begin with

local function do_something(filename)
    local f, err = io.open(filename)
    if not f then return nil, err end
    ...
end

In other words, I have trouble understanding how this use case:

If the user wants to import a schematic in schemedit by typing in the name, the mod currently has to assume the file exists.

isn't already resolved by io.open. You try to open it, if you can't, you report an error to the user. If you want to check whether you can open a file "ahead of time", just refactor your function to take a file handle.

appgurueu avatar Jul 27 '24 14:07 appgurueu

You don't typically open schematics using io.open, you use minetest.read_schematic or minetest.place_schematic with a filename/path. Neither of which distinguish between the file not existing and other errors

rubenwardy avatar Jul 27 '24 14:07 rubenwardy

You don't typically open schematics using io.open, you use minetest.read_schematic or minetest.place_schematic with a filename/path. Neither of which distinguish between the file not existing and other errors

Okay, but then that is a problem of how these functions are designed. They should either (alternatively) accept file handles (which might be a bit tricky to implement) or properly report the error back (probably the way to go).

ps. I see a stronger case for a "stat"/attributes function than for a pure existence check. See e.g. LFS.

pps. sfan's suggestion technically also checks whether the file can be opened to be read (which is usually what you want to check), not whether it just exists. (There might be a discrepancy here especially with mod security.) An alternative workaround would use minetest.get_dir_list on path .. "/.." and look for the file in the list.

appgurueu avatar Jul 27 '24 15:07 appgurueu

(There might be a discrepancy here especially with mod security.)

That would be a bug. Mod security must not allow you to discover the existence of a file that is outside of the whilelist.

sfan5 avatar Jul 27 '24 15:07 sfan5

zipgrep for "file_exists": https://content.minetest.net/zipgrep/d6098467-4bb1-4b73-82b0-9896f7bda385/

rubenwardy avatar Jul 27 '24 15:07 rubenwardy

That would be a bug. Mod security must not allow you to discover the existence of a file that is outside of the whilelist.

Well, it looks to me as if minetest.read_schematic bypasses mod security. So we probably have a minor mod security bug there. See l_mapgen.cpp which ends up calling loadSchematicFromFile with the path (if it is absolute), which just uses this straight in an ifstream (see mg_schematic.cpp).

appgurueu avatar Jul 27 '24 15:07 appgurueu

zipgrep for "file_exists": https://content.minetest.net/zipgrep/d6098467-4bb1-4b73-82b0-9896f7bda385/

Alright, looking at it:

  • The usage in airutils seems appropriate, it just wants to know whether the textures exist, not open them.
  • superflat executes a file in the world path (for configuration), if it exists. This could be reworked to use another API, but dofile is most convenient here.
  • hardcorebrix does the same thing as superflat.
  • signs_lib does the same thing as airutils.
  • painting checks whether files exist to avoid overwriting them. This is a use case I hadn't considered.
  • xmaps doesn't separately check for file existence. There just is a variable that is set depending on whether the handle is nil.
  • lwcomputers implements its own fake filesystem.
  • modinfo uses file_exists unnecessarily in one place. It should just check the file handle returned by io.open instead. In the other place it is necessary since the filename is passed to Settings.
  • ctf_guns, name_generator define it, but don't seem to use it.

at this point I stopped. The use cases for file_exists mostly seem to stem from wanting to ensure the precondition for some API which just works with filenames and doesn't properly report errors to the API user is met (which it is technically insufficient for, but works well enough 99% of the time).

So I am convinced that it is probably a good idea to expose a file_exists as a "hotfix", and for many legitimate use cases where you want to check just whether a file exists and not read it yourself in Lua.

(But ultimately many of our APIs - in this case, schematic APIs - should also properly tell the modder why they couldn't open a file, such that the modder may propagate this to the user.)

appgurueu avatar Jul 27 '24 15:07 appgurueu

2 and a half months later, I second this

Had no idea there was actually a function for checking if a file exists. Saw a friend of mine use io.open() and I was like "is this safe for mod security?" and judging by sfan's response, maybe? So I'll just go with that until there's something like this implemented. Thanks sfan ;p

TubberPupper avatar Oct 04 '24 17:10 TubberPupper