[zip] Simplify ZipReader::ExtractCurrentEntryToString()

The output string is not cleared anymore in case of error. This allows
to observe the extracted content even in case of error, which is useful
in unit tests and consistent with the behaviour of the other Extract*()
methods such as ExtractCurrentEntryToFilePathAsync().

BUG=chromium:1295127
TEST=autoninja -C out/Default zlib_unittests && out/Default/zlib_unittests

Change-Id: I498bdd41f9920b6e5b9d62819540476f23979995
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3473545
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/main@{#973631}
NOKEYCHECK=True
GitOrigin-RevId: da22d8a66a885a419376778e410482678bc9941d

3 files changed, 14 insertions, 27 deletions

diff --git a/google/zip_reader.cc b/google/zip_reader.cc
index dcda7da..ab4476b 100644
--- a/google/zip_reader.cc
+++ b/google/zip_reader.cc
@@ -419,18 +419,7 @@ bool ZipReader::ExtractCurrentEntryToString(uint64_t max_read_bytes,
       max_read_bytes, base::checked_cast<uint64_t>(entry_.original_size))));
 
   StringWriterDelegate writer(output);
-  if (!ExtractCurrentEntry(&writer, max_read_bytes)) {
-    if (output->size() < max_read_bytes) {
-      // There was an error in extracting entry. If ExtractCurrentEntry()
-      // returns false, the entire file was not read - in which case
-      // output->size() should equal |max_read_bytes| unless an error occurred
-      // which caused extraction to be aborted.
-      output->clear();
-    }
-    return false;
-  }
-
-  return true;
+  return ExtractCurrentEntry(&writer, max_read_bytes);
 }
 
 bool ZipReader::OpenInternal() {
diff --git a/google/zip_reader.h b/google/zip_reader.h
index 0d81c21..67cea3c 100644
--- a/google/zip_reader.h
+++ b/google/zip_reader.h
@@ -212,21 +212,18 @@ class ZipReader {
       const ProgressCallback& progress_callback);
 
   // Extracts the current entry into memory. If the current entry is a
-  // directory, the |output| parameter is set to the empty string. If the
-  // current entry is a file, the |output| parameter is filled with its
-  // contents.
+  // directory, |*output| is set to the empty string. If the current entry is a
+  // file, |*output| is filled with its contents.
   //
-  // The |output| parameter can be filled with a big amount of data, avoid
-  // passing it around by value, but by reference or pointer.
-  //
-  // The value in Entry::original_size cannot be trusted, so the real size of
+  // The value in |Entry::original_size| cannot be trusted, so the real size of
   // the uncompressed contents can be different. |max_read_bytes| limits the
-  // ammount of memory used to carry the entry.
+  // amount of memory used to carry the entry.
   //
-  // Returns true if the entire content is read. If the entry is bigger than
-  // |max_read_bytes|, returns false and |output| is filled with
-  // |max_read_bytes| of data. If an error occurs, returns false, and |output|
-  // is set to the empty string.
+  // Returns true if the entire content is read without error. If the content is
+  // bigger than |max_read_bytes|, this function returns false and |*output| is
+  // filled with |max_read_bytes| of data. If an error occurs, this function
+  // returns false and |*output| contains the content extracted so far, which
+  // might be garbage data.
   //
   // Precondition: Next() returned a non-null Entry.
   bool ExtractCurrentEntryToString(uint64_t max_read_bytes,
diff --git a/google/zip_reader_unittest.cc b/google/zip_reader_unittest.cc
index 5cd878d..cb887a9 100644
--- a/google/zip_reader_unittest.cc
+++ b/google/zip_reader_unittest.cc
@@ -442,7 +442,6 @@ TEST_F(ZipReaderTest, EncryptedFile_WrongPassword) {
     EXPECT_TRUE(entry->is_encrypted);
     std::string contents = "dummy";
     EXPECT_FALSE(reader.ExtractCurrentEntryToString(&contents));
-    EXPECT_EQ("", contents);
   }
 
   EXPECT_FALSE(reader.Next());
@@ -872,12 +871,14 @@ TEST_F(ZipReaderTest, WrongCrc) {
 
   std::string contents = "dummy";
   EXPECT_FALSE(reader.ExtractCurrentEntryToString(&contents));
-  EXPECT_EQ("", contents);
+  EXPECT_EQ("This file has been changed after its CRC was computed.\n",
+            contents);
 
   contents = "dummy";
   EXPECT_FALSE(
       reader.ExtractCurrentEntryToString(entry->original_size + 1, &contents));
-  EXPECT_EQ("", contents);
+  EXPECT_EQ("This file has been changed after its CRC was computed.\n",
+            contents);
 
   contents = "dummy";
   EXPECT_FALSE(