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

lib/ZipFile/ZipFile.cpp

@@ -18,6 +18,28 @@ namespace {
 constexpr uint16_t ZIP_METHOD_STORED = 0;
 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) {
   auto* ctx = reinterpret_cast<ZipInflateCtx*>(uncomp);
   if (ctx->fileRemaining == 0) return -1;
@@ -35,17 +57,10 @@ int zipReadCallback(uzlib_uncomp* uncomp) {
 }  // namespace
 
 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);
 
@@ -89,9 +104,6 @@ bool ZipFile::loadAllFileStatSlims() {
   lastCentralDirPos = zipDetails.centralDirOffset;
   lastCentralDirPosValid = true;
 
-  if (!wasOpen) {
-    close();
-  }
   return true;
 }
 
@@ -105,17 +117,10 @@ bool ZipFile::loadFileStatSlim(const char* filename, FileStatSlim* fileStat) {
     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
   uint32_t startPos = lastCentralDirPosValid ? lastCentralDirPos : zipDetails.centralDirOffset;
@@ -179,17 +184,12 @@ bool ZipFile::loadFileStatSlim(const char* filename, FileStatSlim* fileStat) {
     file.seekCur(m + k);
   }
 
-  if (!wasOpen) {
-    close();
-  }
   return found;
 }
 
 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;
 
@@ -198,9 +198,6 @@ long ZipFile::getDataOffset(const FileStatSlim& fileStat) {
 
   file.seek(fileOffset);
   const size_t read = file.read(pLocalHeader, localHeaderSize);
-  if (!wasOpen) {
-    close();
-  }
 
   if (read != localHeaderSize) {
     LOG_ERR("ZIP", "Something went wrong reading the local header");
@@ -223,17 +220,12 @@ bool ZipFile::loadZipDetails() {
     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();
   if (fileSize < 22) {
     LOG_ERR("ZIP", "File too small to be a valid zip");
-    if (!wasOpen) {
-      close();
-    }
     return false;  // Minimum EOCD size is 22 bytes
   }
 
@@ -243,9 +235,6 @@ bool ZipFile::loadZipDetails() {
   const auto buffer = static_cast<uint8_t*>(malloc(scanRange));
   if (!buffer) {
     LOG_ERR("ZIP", "Failed to allocate memory for EOCD scan buffer");
-    if (!wasOpen) {
-      close();
-    }
     return false;
   }
 
@@ -265,9 +254,6 @@ bool ZipFile::loadZipDetails() {
   if (foundOffset == -1) {
     LOG_ERR("ZIP", "EOCD signature not found in zip file");
     free(buffer);
-    if (!wasOpen) {
-      close();
-    }
     return false;
   }
 
@@ -280,9 +266,6 @@ bool ZipFile::loadZipDetails() {
   zipDetails.isSet = true;
 
   free(buffer);
-  if (!wasOpen) {
-    close();
-  }
   return true;
 }
 
@@ -317,17 +300,10 @@ int ZipFile::fillUncompressedSizes(std::vector<SizeTarget>& targets, std::vector
     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);
 
@@ -384,34 +360,18 @@ int ZipFile::fillUncompressedSizes(std::vector<SizeTarget>& targets, std::vector
     file.seekCur(m + k);
   }
 
-  if (!wasOpen) {
-    close();
-  }
-
   return matched;
 }
 
 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 = {};
-  if (!loadFileStatSlim(filename, &fileStat)) {
+  if (!loadFileStatSlim(filename, &fileStat)) return nullptr;
-    if (!wasOpen) {
-      close();
-    }
-    return nullptr;
-  }
 
   const long fileOffset = getDataOffset(fileStat);
-  if (fileOffset < 0) {
+  if (fileOffset < 0) return nullptr;
-    if (!wasOpen) {
-      close();
-    }
-    return nullptr;
-  }
 
   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));
   if (data == nullptr) {
     LOG_ERR("ZIP", "Failed to allocate memory for output buffer (%zu bytes)", dataSize);
-    if (!wasOpen) {
-      close();
-    }
     return nullptr;
   }
 
   if (fileStat.method == ZIP_METHOD_STORED) {
     // no deflation, just read content
     const size_t dataRead = file.read(data, inflatedDataSize);
-    if (!wasOpen) {
-      close();
-    }
 
     if (dataRead != inflatedDataSize) {
       LOG_ERR("ZIP", "Failed to read data");
@@ -446,16 +400,11 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
     const auto deflatedData = static_cast<uint8_t*>(malloc(deflatedDataSize));
     if (deflatedData == nullptr) {
       LOG_ERR("ZIP", "Failed to allocate memory for decompression buffer");
-      if (!wasOpen) {
+      free(data);
-        close();
-      }
       return nullptr;
     }
 
     const size_t dataRead = file.read(deflatedData, deflatedDataSize);
-    if (!wasOpen) {
-      close();
-    }
 
     if (dataRead != deflatedDataSize) {
       LOG_ERR("ZIP", "Failed to read data, expected %d got %d", deflatedDataSize, dataRead);
@@ -482,9 +431,7 @@ uint8_t* ZipFile::readFileToMemory(const char* filename, size_t* size, const boo
     // Continue out of block with data set
   } else {
     LOG_ERR("ZIP", "Unsupported compression method");
-    if (!wasOpen) {
+    free(data);
-      close();
-    }
     return nullptr;
   }
 
@@ -494,20 +441,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) {
-  const bool wasOpen = isOpen();
+  const ScopedOpenClose zip{*this};
-  if (!wasOpen && !open()) {
+  if (!zip) return false;
-    return false;
-  }
 
   FileStatSlim fileStat = {};
-  if (!loadFileStatSlim(filename, &fileStat)) {
+  if (!loadFileStatSlim(filename, &fileStat)) return false;
-    return false;
-  }
 
   const long fileOffset = getDataOffset(fileStat);
-  if (fileOffset < 0) {
+  if (fileOffset < 0) return false;
-    return false;
-  }
 
   file.seek(fileOffset);
   const auto deflatedDataSize = fileStat.compressedSize;
@@ -518,9 +459,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
     const auto buffer = static_cast<uint8_t*>(malloc(chunkSize));
     if (!buffer) {
       LOG_ERR("ZIP", "Failed to allocate memory for buffer");
-      if (!wasOpen) {
-        close();
-      }
       return false;
     }
 
@@ -530,19 +468,17 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
       if (dataRead == 0) {
         LOG_ERR("ZIP", "Could not read more bytes");
         free(buffer);
-        if (!wasOpen) {
-          close();
-        }
         return false;
       }
 
-      out.write(buffer, dataRead);
+      if (out.write(buffer, dataRead) != dataRead) {
+        LOG_ERR("ZIP", "Failed to write all output bytes to stream");
+        free(buffer);
+        return false;
+      }
       remaining -= dataRead;
     }
 
-    if (!wasOpen) {
-      close();
-    }
     free(buffer);
     return true;
   }
@@ -551,9 +487,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
     auto* fileReadBuffer = static_cast<uint8_t*>(malloc(chunkSize));
     if (!fileReadBuffer) {
       LOG_ERR("ZIP", "Failed to allocate memory for zip file read buffer");
-      if (!wasOpen) {
-        close();
-      }
       return false;
     }
 
@@ -561,9 +494,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
     if (!outputBuffer) {
       LOG_ERR("ZIP", "Failed to allocate memory for output buffer");
       free(fileReadBuffer);
-      if (!wasOpen) {
-        close();
-      }
       return false;
     }
 
@@ -577,9 +507,6 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
       LOG_ERR("ZIP", "Failed to init inflate reader");
       free(outputBuffer);
       free(fileReadBuffer);
-      if (!wasOpen) {
-        close();
-      }
       return false;
     }
     ctx.reader.setReadCallback(zipReadCallback);
@@ -623,18 +550,11 @@ bool ZipFile::readFileToStream(const char* filename, Print& out, const size_t ch
       // InflateStatus::Ok: output buffer full, continue
     }
 
-    if (!wasOpen) {
-      close();
-    }
     free(outputBuffer);
     free(fileReadBuffer);
     return success;  // ctx.reader destructor frees the ring buffer
   }
 
-  if (!wasOpen) {
-    close();
-  }
-
   LOG_ERR("ZIP", "Unsupported compression method");
   return false;
 }