Fix buffer over-read and memory leaks when using long filepaths in a zip archive and improved robustness of long filepaths and reading files.

5 files changed, 57 insertions, 41 deletions

diff --git a/modules/zip/doc_classes/ZIPPacker.xml b/modules/zip/doc_classes/ZIPPacker.xml
index 47ea50f031..5d5bedb773 100644
--- a/modules/zip/doc_classes/ZIPPacker.xml
+++ b/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>
diff --git a/modules/zip/zip_packer.cpp b/modules/zip/zip_packer.cpp
index 65f451a3f9..c8b4fb4e77 100644
--- a/modules/zip/zip_packer.cpp
+++ b/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) {
diff --git a/modules/zip/zip_packer.h b/modules/zip/zip_packer.h
index d9d2602d33..142d0fddbf 100644
--- a/modules/zip/zip_packer.h
+++ b/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();
diff --git a/modules/zip/zip_reader.cpp b/modules/zip/zip_reader.cpp
index 898c36a12d..2684875e1c 100644
--- a/modules/zip/zip_reader.cpp
+++ b/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;
 }
 
diff --git a/modules/zip/zip_reader.h b/modules/zip/zip_reader.h
index 8227208eed..c074197eb4 100644
--- a/modules/zip/zip_reader.h
+++ b/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();