Fix leaking file descriptors with zip pack files

[abustin]

I encountered a very large number of open file descriptors for the zip pack file I was using.

$ lsof | grep pack.zip | wc -l
1270

It seems like there is possible recursive behavior related to the use of FileAccess as the implementation of the zlib_filefunc_def function set.

I'm not super familiar with the code. But it seems like FileAccess::open would heap allocates another FileAccess with no owner as well.

// first open
FileAccess *f = FileAccess::open(packages[file.package].filename, FileAccess::READ);

// second open
f->open(p_fname, FileAccess::READ);

Example stack

#0  0x000055555624a660 in FileAccessUnix::_open(String const&, int) ()
#1  0x000055555715304f in FileAccess::open(String const&, int, Error*) ()
#2  0x00005555571edb7c in godot_open ()
#3  0x00005555572ff91d in unzOpenInternal ()
#4  0x0000555557300196 in unzOpen2 ()
#5  0x00005555571edf83 in ZipArchive::get_file_handle(String) const ()
#6  0x00005555571eead0 in ZipArchive::get_file(String const&, PackedData::PackedFile*) ()
#7  0x000055555715315d in FileAccess::open(String const&, int, Error*) ()
#8  0x00005555569f1625 in ResourceFormatLoaderText::load_interactive(String const&, String const&, Error*) ()
#9  0x0000555557249a86 in ResourceFormatLoader::load(String const&, String const&, Error*) ()
#10 0x000055555724f417 in ResourceLoader::load(String const&, String const&, bool, Error*) ()
#11 0x00005555569f4099 in ResourceInteractiveLoaderText::poll() ()
#12 0x0000555557249ae5 in ResourceFormatLoader::load(String const&, String const&, Error*) ()
#13 0x000055555724f417 in ResourceLoader::load(String const&, String const&, bool, Error*) ()
#14 0x0000555555cf8b58 in Main::start() ()
#15 0x0000555555cc260b in main ()

after patch:

$ lsof | grep pack.zip | wc -l
15

Note: I encountered this in Godot 3.2.2 and 3.2.0. I haven't proven this in master (vulkan) since I have a hard time building it.

[akien-mga]

What happens if a file is already open? Won't it fail opening the new file as requested?

[abustin]

It's not clear if it wants to open a different file. We might want to compare the file paths to make sure.

The integration of FileAccess with the uzip lib isn't super clean. It seems like uzip doesn't want to start with an open file, just enough information to do it on its own. But we give it a FileAccess::open anyway. uzip then called the open callback godot_open to open the file, even though we gave it to uzip already opened. godot_open then calls FileAccess::open again and seems to leave it dangling. 😬

We might want to consider starting uzip with FileAccess::create and then call reopen unguarded.

[aaronfranke]

@abustin Is this still desired? If so, it needs to be rebased and tested on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR.

[akien-mga]

I just noticed that there's also this PR which relates to the same code, probably trying to solve a similar issue: #42337. This code is a bit tricky as I don't know any current maintainer who is familiar with this code, so it's hard to assess changes.

[akien-mga]

Superseded by #42337. Thanks for the contribution nevertheless!