refactor: RAII scoped open/close for ZipFile (#1433)

## Summary

**What is the goal of this PR?**

Added `ScopedOpenClose` RAII guard to eliminate repetitive
`wasOpen`/`close()` boilerplate across all ZipFile methods.

---

### AI Usage

While CrossPoint doesn't have restrictions on AI tools in contributing,
please be transparent about their usage as it
helps set the right context for reviewers.

Did you use AI tools to help write this code? _**PARTIALLY**_
This commit is contained in:
Zach Nelson
2026-04-09 21:28:31 +03:00
committed by GitHub
parent 5349e81723
commit b3b43bb373
+43 -129
View File
@@ -18,6 +18,28 @@ namespace {
constexpr uint16_t ZIP_METHOD_STORED = 0; constexpr uint16_t ZIP_METHOD_STORED = 0;
constexpr uint16_t ZIP_METHOD_DEFLATED = 8; constexpr uint16_t ZIP_METHOD_DEFLATED = 8;
// RAII zip: opens the zip if not already open, closes on destruction only if
// it performed the open. Removes the wasOpen/close boilerplate from every method.
class ScopedOpenClose final {
public:
[[nodiscard]] explicit ScopedOpenClose(ZipFile& zf) : zf(zf), needsClose(!zf.isOpen()) {
if (needsClose) ok = zf.open();
}
~ScopedOpenClose() {
if (needsClose && ok) zf.close();
}
ScopedOpenClose(const ScopedOpenClose&) = delete;
ScopedOpenClose& operator=(const ScopedOpenClose&) = delete;
ScopedOpenClose(ScopedOpenClose&&) = delete;
ScopedOpenClose& operator=(ScopedOpenClose&&) = delete;
explicit operator bool() const { return ok || !needsClose; }
private:
ZipFile& zf;
bool needsClose = false;
bool ok = true; // true when zip was already open (no open() call needed)
};
int zipReadCallback(uzlib_uncomp* uncomp) { int zipReadCallback(uzlib_uncomp* uncomp) {
auto* ctx = reinterpret_cast<ZipInflateCtx*>(uncomp); auto* ctx = reinterpret_cast<ZipInflateCtx*>(uncomp);
if (ctx->fileRemaining == 0) return -1; if (ctx->fileRemaining == 0) return -1;
@@ -35,17 +57,10 @@ int zipReadCallback(uzlib_uncomp* uncomp) {
} // namespace } // namespace
bool ZipFile::loadAllFileStatSlims() { bool ZipFile::loadAllFileStatSlims() {
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return false;
return false;
}
if (!loadZipDetails()) { if (!loadZipDetails()) return false;
if (!wasOpen) {
close();
}
return false;
}
file.seek(zipDetails.centralDirOffset); file.seek(zipDetails.centralDirOffset);
@@ -89,9 +104,6 @@ bool ZipFile::loadAllFileStatSlims() {
lastCentralDirPos = zipDetails.centralDirOffset; lastCentralDirPos = zipDetails.centralDirOffset;
lastCentralDirPosValid = true; lastCentralDirPosValid = true;
if (!wasOpen) {
close();
}
return true; return true;
} }
@@ -105,17 +117,10 @@ bool ZipFile::loadFileStatSlim(const char* filename, FileStatSlim* fileStat) {
return false; return false;
} }
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return false;
return false;
}
if (!loadZipDetails()) { if (!loadZipDetails()) return false;
if (!wasOpen) {
close();
}
return false;
}
// Phase 1: Try scanning from cursor position first // Phase 1: Try scanning from cursor position first
uint32_t startPos = lastCentralDirPosValid ? lastCentralDirPos : zipDetails.centralDirOffset; uint32_t startPos = lastCentralDirPosValid ? lastCentralDirPos : zipDetails.centralDirOffset;
@@ -179,17 +184,12 @@ bool ZipFile::loadFileStatSlim(const char* filename, FileStatSlim* fileStat) {
file.seekCur(m + k); file.seekCur(m + k);
} }
if (!wasOpen) {
close();
}
return found; return found;
} }
long ZipFile::getDataOffset(const FileStatSlim& fileStat) { long ZipFile::getDataOffset(const FileStatSlim& fileStat) {
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return -1;
return -1;
}
constexpr auto localHeaderSize = 30; constexpr auto localHeaderSize = 30;
@@ -198,9 +198,6 @@ long ZipFile::getDataOffset(const FileStatSlim& fileStat) {
file.seek(fileOffset); file.seek(fileOffset);
const size_t read = file.read(pLocalHeader, localHeaderSize); const size_t read = file.read(pLocalHeader, localHeaderSize);
if (!wasOpen) {
close();
}
if (read != localHeaderSize) { if (read != localHeaderSize) {
LOG_ERR("ZIP", "Something went wrong reading the local header"); LOG_ERR("ZIP", "Something went wrong reading the local header");
@@ -223,17 +220,12 @@ bool ZipFile::loadZipDetails() {
return true; return true;
} }
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return false;
return false;
}
const size_t fileSize = file.size(); const size_t fileSize = file.size();
if (fileSize < 22) { if (fileSize < 22) {
LOG_ERR("ZIP", "File too small to be a valid zip"); LOG_ERR("ZIP", "File too small to be a valid zip");
if (!wasOpen) {
close();
}
return false; // Minimum EOCD size is 22 bytes return false; // Minimum EOCD size is 22 bytes
} }
@@ -243,9 +235,6 @@ bool ZipFile::loadZipDetails() {
const auto buffer = static_cast<uint8_t*>(malloc(scanRange)); const auto buffer = static_cast<uint8_t*>(malloc(scanRange));
if (!buffer) { if (!buffer) {
LOG_ERR("ZIP", "Failed to allocate memory for EOCD scan buffer"); LOG_ERR("ZIP", "Failed to allocate memory for EOCD scan buffer");
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -265,9 +254,6 @@ bool ZipFile::loadZipDetails() {
if (foundOffset == -1) { if (foundOffset == -1) {
LOG_ERR("ZIP", "EOCD signature not found in zip file"); LOG_ERR("ZIP", "EOCD signature not found in zip file");
free(buffer); free(buffer);
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -280,9 +266,6 @@ bool ZipFile::loadZipDetails() {
zipDetails.isSet = true; zipDetails.isSet = true;
free(buffer); free(buffer);
if (!wasOpen) {
close();
}
return true; return true;
} }
@@ -317,17 +300,10 @@ int ZipFile::fillUncompressedSizes(std::vector<SizeTarget>& targets, std::vector
return 0; return 0;
} }
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return 0;
return 0;
}
if (!loadZipDetails()) { if (!loadZipDetails()) return 0;
if (!wasOpen) {
close();
}
return 0;
}
file.seek(zipDetails.centralDirOffset); file.seek(zipDetails.centralDirOffset);
@@ -384,34 +360,18 @@ int ZipFile::fillUncompressedSizes(std::vector<SizeTarget>& targets, std::vector
file.seekCur(m + k); file.seekCur(m + k);
} }
if (!wasOpen) {
close();
}
return matched; return matched;
} }
uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const bool trailingNullByte) { uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const bool trailingNullByte) {
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return nullptr;
return nullptr;
}
FileStatSlim fileStat = {}; FileStatSlim fileStat = {};
if (!loadFileStatSlim(filename, &fileStat)) { if (!loadFileStatSlim(filename, &fileStat)) return nullptr;
if (!wasOpen) {
close();
}
return nullptr;
}
const long fileOffset = getDataOffset(fileStat); const long fileOffset = getDataOffset(fileStat);
if (fileOffset < 0) { if (fileOffset < 0) return nullptr;
if (!wasOpen) {
close();
}
return nullptr;
}
file.seek(fileOffset); file.seek(fileOffset);
@@ -421,18 +381,12 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
const auto data = static_cast<uint8_t*>(malloc(dataSize)); const auto data = static_cast<uint8_t*>(malloc(dataSize));
if (data == nullptr) { if (data == nullptr) {
LOG_ERR("ZIP", "Failed to allocate memory for output buffer (%zu bytes)", dataSize); LOG_ERR("ZIP", "Failed to allocate memory for output buffer (%zu bytes)", dataSize);
if (!wasOpen) {
close();
}
return nullptr; return nullptr;
} }
if (fileStat.method == ZIP_METHOD_STORED) { if (fileStat.method == ZIP_METHOD_STORED) {
// no deflation, just read content // no deflation, just read content
const size_t dataRead = file.read(data, inflatedDataSize); const size_t dataRead = file.read(data, inflatedDataSize);
if (!wasOpen) {
close();
}
if (dataRead != inflatedDataSize) { if (dataRead != inflatedDataSize) {
LOG_ERR("ZIP", "Failed to read data"); LOG_ERR("ZIP", "Failed to read data");
@@ -446,16 +400,10 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
const auto deflatedData = static_cast<uint8_t*>(malloc(deflatedDataSize)); const auto deflatedData = static_cast<uint8_t*>(malloc(deflatedDataSize));
if (deflatedData == nullptr) { if (deflatedData == nullptr) {
LOG_ERR("ZIP", "Failed to allocate memory for decompression buffer"); LOG_ERR("ZIP", "Failed to allocate memory for decompression buffer");
if (!wasOpen) {
close();
}
return nullptr; return nullptr;
} }
const size_t dataRead = file.read(deflatedData, deflatedDataSize); const size_t dataRead = file.read(deflatedData, deflatedDataSize);
if (!wasOpen) {
close();
}
if (dataRead != deflatedDataSize) { if (dataRead != deflatedDataSize) {
LOG_ERR("ZIP", "Failed to read data, expected %d got %d", deflatedDataSize, dataRead); LOG_ERR("ZIP", "Failed to read data, expected %d got %d", deflatedDataSize, dataRead);
@@ -482,9 +430,6 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
// Continue out of block with data set // Continue out of block with data set
} else { } else {
LOG_ERR("ZIP", "Unsupported compression method"); LOG_ERR("ZIP", "Unsupported compression method");
if (!wasOpen) {
close();
}
return nullptr; return nullptr;
} }
@@ -494,20 +439,14 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
} }
bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize) { bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t chunkSize) {
const bool wasOpen = isOpen(); const ScopedOpenClose zip{*this};
if (!wasOpen && !open()) { if (!zip) return false;
return false;
}
FileStatSlim fileStat = {}; FileStatSlim fileStat = {};
if (!loadFileStatSlim(filename, &fileStat)) { if (!loadFileStatSlim(filename, &fileStat)) return false;
return false;
}
const long fileOffset = getDataOffset(fileStat); const long fileOffset = getDataOffset(fileStat);
if (fileOffset < 0) { if (fileOffset < 0) return false;
return false;
}
file.seek(fileOffset); file.seek(fileOffset);
const auto deflatedDataSize = fileStat.compressedSize; const auto deflatedDataSize = fileStat.compressedSize;
@@ -518,9 +457,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
const auto buffer = static_cast<uint8_t*>(malloc(chunkSize)); const auto buffer = static_cast<uint8_t*>(malloc(chunkSize));
if (!buffer) { if (!buffer) {
LOG_ERR("ZIP", "Failed to allocate memory for buffer"); LOG_ERR("ZIP", "Failed to allocate memory for buffer");
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -530,9 +466,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
if (dataRead == 0) { if (dataRead == 0) {
LOG_ERR("ZIP", "Could not read more bytes"); LOG_ERR("ZIP", "Could not read more bytes");
free(buffer); free(buffer);
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -540,9 +473,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
remaining -= dataRead; remaining -= dataRead;
} }
if (!wasOpen) {
close();
}
free(buffer); free(buffer);
return true; return true;
} }
@@ -551,9 +481,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
auto* fileReadBuffer = static_cast<uint8_t*>(malloc(chunkSize)); auto* fileReadBuffer = static_cast<uint8_t*>(malloc(chunkSize));
if (!fileReadBuffer) { if (!fileReadBuffer) {
LOG_ERR("ZIP", "Failed to allocate memory for zip file read buffer"); LOG_ERR("ZIP", "Failed to allocate memory for zip file read buffer");
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -561,9 +488,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
if (!outputBuffer) { if (!outputBuffer) {
LOG_ERR("ZIP", "Failed to allocate memory for output buffer"); LOG_ERR("ZIP", "Failed to allocate memory for output buffer");
free(fileReadBuffer); free(fileReadBuffer);
if (!wasOpen) {
close();
}
return false; return false;
} }
@@ -577,9 +501,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
LOG_ERR("ZIP", "Failed to init inflate reader"); LOG_ERR("ZIP", "Failed to init inflate reader");
free(outputBuffer); free(outputBuffer);
free(fileReadBuffer); free(fileReadBuffer);
if (!wasOpen) {
close();
}
return false; return false;
} }
ctx.reader.setReadCallback(zipReadCallback); ctx.reader.setReadCallback(zipReadCallback);
@@ -623,18 +544,11 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
// InflateStatus::Ok: output buffer full, continue // InflateStatus::Ok: output buffer full, continue
} }
if (!wasOpen) {
close();
}
free(outputBuffer); free(outputBuffer);
free(fileReadBuffer); free(fileReadBuffer);
return success; // ctx.reader destructor frees the ring buffer return success; // ctx.reader destructor frees the ring buffer
} }
if (!wasOpen) {
close();
}
LOG_ERR("ZIP", "Unsupported compression method"); LOG_ERR("ZIP", "Unsupported compression method");
return false; return false;
} }