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

FileSystem: implement symlink support in zip archives, fix #83 #176

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
3 participants
@illwieckz
Copy link
Member

illwieckz commented Mar 3, 2019

  • original C++ and BSD zip symlink resolver
  • only load zip symlink from the same pak
  • optional fs_pakSymlink cvar to enable the feature (disabled by default)

See Darkplaces reference implementation:
https://gitlab.com/xonotic/darkplaces/commit/1c55fd95ff34d9aafb76c927215ea8fbab5210c6
No code from Darkplaces is copypasted to not taint with GPLv2+.

@illwieckz illwieckz added this to To do in Xonotic via automation Mar 3, 2019

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from ae10387 to 1a847e2 Mar 3, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 3, 2019

It produces useful debug log like that:

Debug: found symlink: dds/textures/trak5x/floor/floor_floor2a.dds → dds/textures/trak5x/floor/floor_floor2a_gloss.dds 

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from 1a847e2 to a875abe Mar 3, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 3, 2019

This trick is meant to be supported even on systems that do not provide S_ISLNK:

		uLong attr = fileInfo.external_fa >> 16;
		return ( ( attr & 0120000 ) == 0120000 );

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from a875abe to f385cbb Mar 3, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 3, 2019

Note that I can also import FS_CheckNastyPath from DarkPlaces, it may be useful, and not only for dpk/pk3:
https://gitlab.com/xonotic/darkplaces/blob/cf8dfc912d76d0a26c5d80202fb0b8089df0901f/fs.c#L2439-2494

Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp Outdated
@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 4, 2019

This appears to allow symlinks to refer to arbitrary files anywhere in the filesystem which seems really gross to me. If symlinks are really needed, they should only be able to refer to files in the same zip package.

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from f385cbb to 63cdf20 Mar 5, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

I updated the code with constants, and made the feature disabled by default.
A cvar named fs_pakSymlink can be set to true to enable the feature.

@slipher wrote:

This appears to allow symlinks to refer to arbitrary files anywhere in the filesystem which seems really gross to me. If symlinks are really needed, they should only be able to refer to files in the same zip package.

Basically this feature has only one purpose: provide backward compatibility with Darkplaces, and unless I'm wrong it looks like Darkplaces is doing it the cross-pk3 way, but perhaps I missed something. Xonotic uses this feature to deduplicate files: since they rely on automatic normal/specular/glow map detection based on file name, they have to provide a working filename for every texture, the also deduplicate sounds and other stuff. Basically they have almost 2K symlinks in their pk3. I have not yet checked if they were really using them accross pk3 or not.

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from 63cdf20 to 35eb5f8 Mar 5, 2019

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 5, 2019

How would it make sense to have e.g. the normalmap and the main texture in different packages? The Darkplaces implementation may permit that, but it's a misfeature that should be avoided if at at all possible.

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from 35eb5f8 to c411b7c Mar 10, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 10, 2019

The code now only looks for symlink in the same pak.
As an extra benefit it would load the zip only once.

This is based on the assumption fileMap.find will find the same pak for the target path.
I don't know if the assumption is correct, if not, the problem is probably hardly decidable or undecidable with the current filesystem.

Show resolved Hide resolved src/common/FileSystem.cpp Outdated
Show resolved Hide resolved src/common/FileSystem.cpp

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch 4 times, most recently from 6f351ae to 1206aad Mar 10, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 11, 2019

So, to sum up some talks, my first motivation into this and into any Darkplaces compatibility layer is to use Xonotic maps as test bed for Dæmon, given

  1. they were not tested on Dæmon at design time hence not designed to workaround Dæmon bugs;
  2. they are very polished ones with a whole community able to contribute to refine every detail (they have public map repositories and on-commit build bot since years);
  3. they rely on features we don't use yet and that are likely to be broken (parallax, water…);
  4. they rely on features we don't implement yet (sRGB…).

Everything of this make Xonotic maps a very good test bed, and probably the better one available out there, this test bed breaks Dæmon in many ways and sets a minimum feature and quality level for Dæmon to target. I'm intimately convinced that Dæmon must reach graphical feature parity and graphical quality parity with Darkplaces, not because of Xonotic but because of Unvanquished: we need a better engine. In the past Dæmon looked sexy but in 2019 it just looks like Quake3 with normal maps.

On the zip symlink topic, after a deep thought I now think that it's a bad idea to restrain the zip symlink to one pak, in fact it defeat the whole purpose of pk3/dpk: being able to overload an existing vfs. I'm also intimately convinced that putting symlinks in pk3 was a bad idea at first, but that's too late. It's still a very efficient solution since pkzip format is not a solid archive and does not deduplicate, but it's meh. So, the purpose of this PR is not to encourage people to symlink stuff neither to say it's a good idea. The purpose of this PR is to make Dæmon able to load the best test bed we can put our hands on.

That's also why I think that this PR makes no sense if it does not target 1:1 behavior. That's also why, with the cross-pak symlink support, I ported the Darkplaces code to solve those symlinks. The purpose of this PR is not to provide symlink support to Dæmon based games (my opinion is to seriously discourage this), the purpose of this PR is to provide the test bed map the algorithm they expect. So, even if I would like to see a more c++ey symlink solver I think it's better to keep it at it because we know the test bed was built to work on that algorithm.

Note that we may display a warning just telling people that pak symlink is enabled, just to tell that's creepy.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 11, 2019

But I'm very concerned by the addition of a GPLv2+ code into a BSD one (the symlink resolver). That's enough to me to ask for a rewrite.

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch 5 times, most recently from 18d493e to 2c579b9 Mar 12, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 12, 2019

I tried to do the symlink stuff within the zipFile class, then after a lot of trouble I just thought it was a bad idea from the start. What makes me think it's a bad idea is that it would be a very bad design choice to implement this whithin the zipFile code if we were supporting more than one archive format. So, even if we only support one archive format, it looks like doing it in the pak abstraction is the best place.

It looks like fileMap is a std::unordered_map which has unique keys. Hence testing if the pak providing the file listed in fileMap is the same pak containing the symlink does the expected job: it will not bypass DEPS.

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch 5 times, most recently from d1780f9 to 168a189 Mar 12, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 12, 2019

I added extra comments to explain how it works.

FileSystem: implement symlink support in zip archives, fix #83
- original C++ and BSD zip symlink resolver
- only load zip symlink from the same pak
- optional fs_pakSymlink cvar to enable the feature (disabled by default)

@illwieckz illwieckz force-pushed the illwieckz:zipsymlink branch from 168a189 to be39dc4 Mar 12, 2019

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 15, 2019

Alternative implementation: #182

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 16, 2019

#182 looks better so I'm closing this one.

@illwieckz illwieckz closed this Mar 16, 2019

Xonotic automation moved this from To do to Done Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.