Fix buffer over-read and memory leaks when using long filepaths in minizip API

core/io/zip_io.cpp

@@ -30,6 +30,48 @@
 
 #include "zip_io.h"
 
+#include "core/templates/local_vector.h"
+
+int godot_unzip_get_current_file_info(unzFile p_zip_file, unz_file_info64 &r_file_info, String &r_filepath) {
+	const uLong short_file_path_buffer_size = 16384ul;
+	char short_file_path_buffer[short_file_path_buffer_size];
+
+	int err = unzGetCurrentFileInfo64(p_zip_file, &r_file_info, short_file_path_buffer, short_file_path_buffer_size, nullptr, 0, nullptr, 0);
+	if (unlikely((err != UNZ_OK) || (r_file_info.size_filename > short_file_path_buffer_size))) {
+		LocalVector<char> long_file_path_buffer;
+		long_file_path_buffer.resize(r_file_info.size_filename);
+
+		err = unzGetCurrentFileInfo64(p_zip_file, &r_file_info, long_file_path_buffer.ptr(), long_file_path_buffer.size(), nullptr, 0, nullptr, 0);
+		if (err != UNZ_OK) {
+			return err;
+		}
+		r_filepath = String::utf8(long_file_path_buffer.ptr(), r_file_info.size_filename);
+	} else {
+		r_filepath = String::utf8(short_file_path_buffer, r_file_info.size_filename);
+	}
+
+	return err;
+}
+
+int godot_unzip_locate_file(unzFile p_zip_file, String p_filepath, bool p_case_sensitive) {
+	int err = unzGoToFirstFile(p_zip_file);
+	while (err == UNZ_OK) {
+		unz_file_info64 current_file_info;
+		String current_filepath;
+		err = godot_unzip_get_current_file_info(p_zip_file, current_file_info, current_filepath);
+		if (err == UNZ_OK) {
+			bool filepaths_are_equal = p_case_sensitive ? (p_filepath == current_filepath) : (p_filepath.nocasecmp_to(current_filepath) == 0);
+			if (filepaths_are_equal) {
+				return UNZ_OK;
+			}
+			err = unzGoToNextFile(p_zip_file);
+		}
+	}
+	return err;
+}
+
+//
+
 void *zipio_open(voidpf opaque, const char *p_fname, int mode) {
 	Ref<FileAccess> *fa = reinterpret_cast<Ref<FileAccess> *>(opaque);
 	ERR_FAIL_COND_V(fa == nullptr, nullptr);
@@ -38,17 +80,17 @@ void *zipio_open(voidpf opaque, const char *p_fname, int mode) {
 	fname.parse_utf8(p_fname);
 
 	int file_access_mode = 0;
-	if (mode & ZLIB_FILEFUNC_MODE_WRITE) {
-		file_access_mode |= FileAccess::WRITE;
-	}
 	if (mode & ZLIB_FILEFUNC_MODE_READ) {
 		file_access_mode |= FileAccess::READ;
 	}
+	if (mode & ZLIB_FILEFUNC_MODE_WRITE) {
+		file_access_mode |= FileAccess::WRITE;
+	}
 	if (mode & ZLIB_FILEFUNC_MODE_CREATE) {
 		file_access_mode |= FileAccess::WRITE_READ;
 	}
-	(*fa) = FileAccess::open(fname, file_access_mode);
 
+	(*fa) = FileAccess::open(fname, file_access_mode);
 	if (fa->is_null()) {
 		return nullptr;
 	}

core/io/zip_io.h

@@ -39,6 +39,13 @@
 #include "thirdparty/minizip/unzip.h"
 #include "thirdparty/minizip/zip.h"
 
+// Get the current file info and safely convert the full filepath to a String.
+int godot_unzip_get_current_file_info(unzFile p_zip_file, unz_file_info64 &r_file_info, String &r_filepath);
+// Try to locate the file in the archive specified by the filepath (works with large paths and Unicode).
+int godot_unzip_locate_file(unzFile p_zip_file, String p_filepath, bool p_case_sensitive = true);
+
+//
+
 void *zipio_open(voidpf opaque, const char *p_fname, int mode);
 uLong zipio_read(voidpf opaque, voidpf stream, void *buf, uLong size);
 uLong zipio_write(voidpf opaque, voidpf stream, const void *buf, uLong size);

modules/zip/doc_classes/ZIPPacker.xml

@@ -63,10 +63,13 @@
 	</methods>
 	<constants>
 		<constant name="APPEND_CREATE" value="0" enum="ZipAppend">
+			Create a new zip archive at the given path.
 		</constant>
 		<constant name="APPEND_CREATEAFTER" value="1" enum="ZipAppend">
+			Append a new zip archive to the end of the already existing file at the given path.
 		</constant>
 		<constant name="APPEND_ADDINZIP" value="2" enum="ZipAppend">
+			Add new files to the existing zip archive at the given path.
 		</constant>
 	</constants>
 </class>

modules/zip/zip_packer.cpp

@@ -39,17 +39,19 @@ Error ZIPPacker::open(String p_path, ZipAppend p_append) {
 	}
 
 	zlib_filefunc_def io = zipio_create_io(&fa);
-	zf = zipOpen2(p_path.utf8().get_data(), p_append, NULL, &io);
-	return zf != NULL ? OK : FAILED;
+	zf = zipOpen2(p_path.utf8().get_data(), p_append, nullptr, &io);
+	return zf != nullptr ? OK : FAILED;
 }
 
 Error ZIPPacker::close() {
 	ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPPacker cannot be closed because it is not open.");
 
-	Error err = zipClose(zf, NULL) == ZIP_OK ? OK : FAILED;
+	Error err = zipClose(zf, nullptr) == ZIP_OK ? OK : FAILED;
 	if (err == OK) {
-		zf = NULL;
+		DEV_ASSERT(fa == nullptr);
+		zf = nullptr;
 	}
+
 	return err;
 }
 
@@ -60,18 +62,18 @@ Error ZIPPacker::start_file(String p_path) {
 
 	OS::DateTime time = OS::get_singleton()->get_datetime();
 
+	zipfi.tmz_date.tm_sec = time.second;
+	zipfi.tmz_date.tm_min = time.minute;
 	zipfi.tmz_date.tm_hour = time.hour;
 	zipfi.tmz_date.tm_mday = time.day;
-	zipfi.tmz_date.tm_min = time.minute;
 	zipfi.tmz_date.tm_mon = time.month - 1;
-	zipfi.tmz_date.tm_sec = time.second;
 	zipfi.tmz_date.tm_year = time.year;
 	zipfi.dosDate = 0;
-	zipfi.external_fa = 0;
 	zipfi.internal_fa = 0;
+	zipfi.external_fa = 0;
 
-	int ret = zipOpenNewFileInZip(zf, p_path.utf8().get_data(), &zipfi, NULL, 0, NULL, 0, NULL, Z_DEFLATED, Z_DEFAULT_COMPRESSION);
-	return ret == ZIP_OK ? OK : FAILED;
+	int err = zipOpenNewFileInZip(zf, p_path.utf8().get_data(), &zipfi, nullptr, 0, nullptr, 0, nullptr, Z_DEFLATED, Z_DEFAULT_COMPRESSION);
+	return err == ZIP_OK ? OK : FAILED;
 }
 
 Error ZIPPacker::write_file(Vector<uint8_t> p_data) {

modules/zip/zip_packer.h

@@ -40,7 +40,7 @@ class ZIPPacker : public RefCounted {
 	GDCLASS(ZIPPacker, RefCounted);
 
 	Ref<FileAccess> fa;
-	zipFile zf;
+	zipFile zf = nullptr;
 
 protected:
 	static void _bind_methods();

modules/zip/zip_reader.cpp

@@ -40,38 +40,35 @@ Error ZIPReader::open(String p_path) {
 
 	zlib_filefunc_def io = zipio_create_io(&fa);
 	uzf = unzOpen2(p_path.utf8().get_data(), &io);
-	return uzf != NULL ? OK : FAILED;
+	return uzf != nullptr ? OK : FAILED;
 }
 
 Error ZIPReader::close() {
 	ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPReader cannot be closed because it is not open.");
 
-	return unzClose(uzf) == UNZ_OK ? OK : FAILED;
+	Error err = unzClose(uzf) == UNZ_OK ? OK : FAILED;
+	if (err == OK) {
+		DEV_ASSERT(fa == nullptr);
+		uzf = nullptr;
+	}
+
+	return err;
 }
 
 PackedStringArray ZIPReader::get_files() {
 	ERR_FAIL_COND_V_MSG(fa.is_null(), PackedStringArray(), "ZIPReader must be opened before use.");
 
-	List<String> s;
-
-	if (unzGoToFirstFile(uzf) != UNZ_OK) {
-		return PackedStringArray();
-	}
+	int err = unzGoToFirstFile(uzf);
+	ERR_FAIL_COND_V(err != UNZ_OK, PackedStringArray());
 
+	List<String> s;
 	do {
 		unz_file_info64 file_info;
-		char filename[256]; // Note filename is a path !
-		int err = unzGetCurrentFileInfo64(uzf, &file_info, filename, sizeof(filename), NULL, 0, NULL, 0);
+		String filepath;
+
+		err = godot_unzip_get_current_file_info(uzf, file_info, filepath);
 		if (err == UNZ_OK) {
-			s.push_back(filename);
-		} else {
-			// Assume filename buffer was too small
-			char *long_filename_buff = (char *)memalloc(file_info.size_filename);
-			int err2 = unzGetCurrentFileInfo64(uzf, NULL, long_filename_buff, sizeof(long_filename_buff), NULL, 0, NULL, 0);
-			if (err2 == UNZ_OK) {
-				s.push_back(long_filename_buff);
-				memfree(long_filename_buff);
-			}
+			s.push_back(filepath);
 		}
 	} while (unzGoToNextFile(uzf) == UNZ_OK);
 
@@ -87,23 +84,37 @@ PackedStringArray ZIPReader::get_files() {
 PackedByteArray ZIPReader::read_file(String p_path, bool p_case_sensitive) {
 	ERR_FAIL_COND_V_MSG(fa.is_null(), PackedByteArray(), "ZIPReader must be opened before use.");
 
-	int cs = p_case_sensitive ? 1 : 2;
-	if (unzLocateFile(uzf, p_path.utf8().get_data(), cs) != UNZ_OK) {
-		ERR_FAIL_V_MSG(PackedByteArray(), "File does not exist in zip archive: " + p_path);
-	}
-	if (unzOpenCurrentFile(uzf) != UNZ_OK) {
-		ERR_FAIL_V_MSG(PackedByteArray(), "Could not open file within zip archive.");
-	}
+	int err = UNZ_OK;
+
+	// Locate and open the file.
+	err = godot_unzip_locate_file(uzf, p_path, p_case_sensitive);
+	ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "File does not exist in zip archive: " + p_path);
+	err = unzOpenCurrentFile(uzf);
+	ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "Could not open file within zip archive.");
 
+	// Read the file info.
 	unz_file_info info;
-	unzGetCurrentFileInfo(uzf, &info, NULL, 0, NULL, 0, NULL, 0);
+	err = unzGetCurrentFileInfo(uzf, &info, nullptr, 0, nullptr, 0, nullptr, 0);
+	ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "Unable to read file information from zip archive.");
+	ERR_FAIL_COND_V_MSG(info.uncompressed_size > INT_MAX, PackedByteArray(), "File contents too large to read from zip archive (>2 GB).");
+
+	// Read the file data.
 	PackedByteArray data;
 	data.resize(info.uncompressed_size);
+	uint8_t *buffer = data.ptrw();
+	int to_read = data.size();
+	while (to_read > 0) {
+		int bytes_read = unzReadCurrentFile(uzf, buffer, to_read);
+		ERR_FAIL_COND_V_MSG(bytes_read < 0, PackedByteArray(), "IO/zlib error reading file from zip archive.");
+		ERR_FAIL_COND_V_MSG(bytes_read == UNZ_EOF && to_read != 0, PackedByteArray(), "Incomplete file read from zip archive.");
+		DEV_ASSERT(bytes_read <= to_read);
+		buffer += bytes_read;
+		to_read -= bytes_read;
+	}
 
-	uint8_t *w = data.ptrw();
-	unzReadCurrentFile(uzf, &w[0], info.uncompressed_size);
-
-	unzCloseCurrentFile(uzf);
+	// Verify the data and return.
+	err = unzCloseCurrentFile(uzf);
+	ERR_FAIL_COND_V_MSG(err != UNZ_OK, PackedByteArray(), "CRC error reading file from zip archive.");
 	return data;
 }
 

modules/zip/zip_reader.h

@@ -40,7 +40,7 @@ class ZIPReader : public RefCounted {
 	GDCLASS(ZIPReader, RefCounted)
 
 	Ref<FileAccess> fa;
-	unzFile uzf;
+	unzFile uzf = nullptr;
 
 protected:
 	static void _bind_methods();