[Rseding91] [0.17.23] Factorio deletes files outside the factorio directory

[mrvn]

A while back when discussing hot to best maintain mods in git and play with them someone suggested placing a symlink in factorio/mods/name_version -> somewhere_else/name. That way one doesn't have to constantly rename the git repository but only change the link.

But when I accidentally updated the mod I had linked that way in game factorio happily went and followed the symlink and deleted everything recursively. So the whole git repository OUTSIDE the factorio dir is gone. Now imagine someone creates a symlink to /. I could include that in my evil_mod.zip and suggest to people they should unzip it to improve loading times. Next time factorio updates the mod the whole system gets deleted.

Please use lstat() instead of stat() when checking for directories and ensure that symlinks don't leave the factorio directory. Probably best to not follow any symlink to outside of the base directory of a mod ( factorio/mods/name_version) or one mod could still delete another mods files.

[Rseding91]

Please use lstat() instead of stat() when checking for directories and ensure that symlinks don't leave the factorio directory.

We don't use either. We use <filesystem> https://en.cppreference.com/w/cpp/header/filesystem

[mrvn]

Please use lstat() instead of stat() when checking for directories and ensure that symlinks don't leave the factorio directory.

We don't use either. We use <filesystem> https://en.cppreference.com/w/cpp/header/filesystem

Then that would be symlink_status instead of status(). And I guess the recursive_directory_iterator is out.

[Rseding91]

Ok, after talking with Oxyd about the solution it's now fixed for the next version of 0.17.

[quyxkh]

Please use lstat() instead of stat() when checking for directories and ensure that symlinks don't leave the factorio directory.

We don't use either. We use <filesystem> https://en.cppreference.com/w/cpp/header/filesystem

According to fs.op.remove_all,

27.10.15.31 Remove all [fs.op.remove_all]
uintmax_t remove_all(const path& p);
uintmax_t remove_all(const path& p, error_code& ec) noexcept;

• Effects: Recursively deletes the contents of p if it exists, then deletes file p itself, as if by POSIX
  remove(). [ Note: A symbolic link is itself removed, rather than the file it resolves to. — end note ]
• Postcondition: !exists(symlink_status(p)).
• Returns: The number of files removed. The signature with argument ec returns static_cast<
  uintmax_t>(-1) if an error occurs.
• Throws: As specified in Error reporting (27.10.7).

and I get the same behavior on Linux, installing a mod from the portal doesn't just delete the mod directory's installed symlink to my development repo, it deletes my development repo.

So the only way I can see to get that is you're removing the contents _and_ the entry, rather than just remove_all'ing the entry.

[mrvn]

Please use lstat() instead of stat() when checking for directories and ensure that symlinks don't leave the factorio directory.

We don't use either. We use <filesystem> https://en.cppreference.com/w/cpp/header/filesystem

According to fs.op.remove_all,

27.10.15.31 Remove all [fs.op.remove_all]
uintmax_t remove_all(const path& p);
uintmax_t remove_all(const path& p, error_code& ec) noexcept;

• Effects: Recursively deletes the contents of p if it exists, then deletes file p itself, as if by POSIX
  remove(). [ Note: A symbolic link is itself removed, rather than the file it resolves to. — end note ]
• Postcondition: !exists(symlink_status(p)).
• Returns: The number of files removed. The signature with argument ec returns static_cast<
  uintmax_t>(-1) if an error occurs.
• Throws: As specified in Error reporting (27.10.7).

and I get the same behavior on Linux, installing a mod from the portal doesn't just delete the mod directory's installed symlink to my development repo, it deletes my development repo.

So the only way I can see to get that is you're removing the contents _and_ the entry, rather than just remove_all'ing the entry.

The problem is

Recursively deletes the contents of p

. p is a symlink to a directory so all the contents are deleted recursively and then the symlink but not what the symlink points too. Imho it should not recurse into symlinks or have a flag for that.

[quyxkh]

The problem is […] p is a symlink to a directory so all the contents are deleted recursively and then the symlink but not what the symlink points too. Imho it should not recurse into symlinks or have a flag for that.

Well, they fixed it, but I'm curious: what evidence were you looking at that led you to say so?

Code: Select all

#include <filesystem>
int main(int c, char **v)
{
	using namespace std::filesystem;
	create_directories("testdir/testcontents");
	create_directory_symlink("testdir","testlink");
	remove_all("testlink");
	return 0;
}

deletes the symlink and leaves the directories behind, as required, so I don't see what led you to say what you did.

[Therax]

I've had the same experience, but I'm on Windows. Windows "symlink" story is so confused that I wouldn't expect filesystem to get it right in all cases. Starting Factorio with a symlink in the mods directory pointing to a git repository, and then updating the mod via the in-game interface downloads the latest version from the mod portal in zip format, and then proceeds to recursively remove everything in the git repository directory, and finally remove the symlink.

[mrvn]

The problem is […] p is a symlink to a directory so all the contents are deleted recursively and then the symlink but not what the symlink points too. Imho it should not recurse into symlinks or have a flag for that.

Well, they fixed it, but I'm curious: what evidence were you looking at that led you to say so?

Code: Select all

#include <filesystem>
int main(int c, char **v)
{
	using namespace std::filesystem;
	create_directories("testdir/testcontents");
	create_directory_symlink("testdir","testlink");
	remove_all("testlink");
	return 0;
}

deletes the symlink and leaves the directories behind, as required, so I don't see what led you to say what you did.

Factorio didn't leave the subdirs behind. And if factorio was using remove_all ... Do you have the same STL library as factorio is using? Maybe they fixed the STL? Or factorio wasn't yet using remove_all but is now?