zipAbandonFileInZip reports success when nothing was truncated

src/Makefile.am

@@ -112,6 +112,8 @@ EXTRA_DIST = httrack.h htsstats.h webhttrack.in \
 		coucal/murmurhash3.h.orig \
 		minizip/iowin32.c \
 		minizip/iowin32.h \
+		minizip/iowin32.c.diff \
+		minizip/iowin32.c.orig \
 		minizip/ioapi.c.diff \
 		minizip/ioapi.h.diff \
 		minizip/zip.c.diff \

src/htscache.c

@@ -184,8 +184,12 @@ static void cache_zip_write_failed(httrackp *opt, cache_back *cache,
 
   /* Roll the partial member back: closing it would commit a short body under
      the X-Size already written into its local header. */
-  if (entry_open)
-    (void) zipAbandonFileInZip((zipFile) cache->zipOutput);
+  if (entry_open && zipAbandonFileInZip((zipFile) cache->zipOutput) != ZIP_OK) {
+    /* the bytes are still past the rewound position, and a tail longer than a
+       reader's backscan hides the directory the close writes after it */
+    hts_log_print(opt, LOG_WARNING,
+                  "cache rollback incomplete, the cache file may not reopen");
+  }
   cache->zipWriteFailures++;
   if (fatal_errno || cache->zipWriteFailures >= CACHE_MAX_WRITE_FAILURES) {
     if (!cache->zipWriteFailed) {

src/htscache_selftest.c

@@ -381,6 +381,8 @@ typedef struct {
   int fail_errno; /**< errno set on the failing write (ENOSPC, EIO, ...) */
   int writes;     /**< zwrite call count, to detect re-entry into the stream */
   int fail_once;  /**< recover (unlimited budget) after the first failure */
+  int truncates;  /**< ztruncate call count, to prove the rollback truncates */
+  truncate64_file_func truncate; /**< the backend's own, NULL if it has none */
 } writefail_inject;
 
 /* zwrite that copies until the budget runs out, then fails with inj->fail_errno
@@ -401,16 +403,31 @@ static uLong selftest_failing_zwrite(voidpf opaque, voidpf stream,
   return 0; /* short write -> the minizip op returns an error */
 }
 
+/* Count the truncate the rolled-back entry goes through, then run it. */
+static int ZCALLBACK selftest_counting_ztruncate(voidpf opaque, voidpf stream,
+                                                 ZPOS64_T size) {
+  writefail_inject *inj = (writefail_inject *) opaque;
+
+  inj->truncates++;
+  if (inj->truncate == NULL)
+    return -1;
+  return inj->truncate(opaque, stream, size);
+}
+
 /* Open a ZIP whose writes fail past inj->budget, so cache_add() hits an error.
- */
+   Through the table the cache itself opens with (#1402), or with its truncate
+   entry dropped, as the Win32 tables were: a rollback then only rewinds. */
 static zipFile selftest_open_failing_zip(const char *path,
-                                         writefail_inject *inj) {
-  zlib_filefunc_def ff;
+                                         writefail_inject *inj,
+                                         hts_boolean truncatable) {
+  zlib_filefunc64_def ff;
 
-  fill_fopen_filefunc(&ff); /* real fopen/read/seek/close; ignores opaque */
+  hts_zip_filefunc64(&ff); /* real fopen/read/seek/close; ignores opaque */
+  inj->truncate = truncatable ? ff.ztruncate64_file : NULL;
+  ff.ztruncate64_file = truncatable ? selftest_counting_ztruncate : NULL;
   ff.zwrite_file = selftest_failing_zwrite;
   ff.opaque = inj;
-  return zipOpen2(path, APPEND_STATUS_CREATE, NULL, &ff);
+  return zipOpen2_64(path, APPEND_STATUS_CREATE, NULL, &ff);
 }
 
 /* Store one octet-stream body into `cache` (all-in-cache, body in the ZIP). */
@@ -501,13 +518,14 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
     inj.budget = (phase == 0) ? 4096 : 0;
     inj.fail_errno = (phase == 0) ? ENOSPC : EIO;
     inj.writes = 0;
+    inj.truncates = 0;
     inj.fail_once = 0;
     memset(&cache, 0, sizeof(cache));
     cache.type = 1;
     cache.log = stderr;
     cache.errlog = stderr;
     cache.hashtable = coucal_new(0);
-    cache.zipOutput = selftest_open_failing_zip(path, &inj);
+    cache.zipOutput = selftest_open_failing_zip(path, &inj, HTS_TRUE);
     if (cache.zipOutput == NULL) {
       fprintf(stderr, "cache-writefail: could not open injected ZIP\n");
       fail++;
@@ -573,13 +591,14 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
     inj.budget = (size_t) -1;
     inj.fail_errno = EIO;
     inj.writes = 0;
+    inj.truncates = 0;
     inj.fail_once = 0;
     memset(&cache, 0, sizeof(cache));
     cache.type = 1;
     cache.log = stderr;
     cache.errlog = stderr;
     cache.hashtable = coucal_new(0);
-    cache.zipOutput = selftest_open_failing_zip(path, &inj);
+    cache.zipOutput = selftest_open_failing_zip(path, &inj, HTS_TRUE);
     opt->state.exit_xh = 0;
 
     for (i = 0; i < 10; i++) {
@@ -614,13 +633,14 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
     inj.budget = 4096;
     inj.fail_errno = EIO;
     inj.writes = 0;
+    inj.truncates = 0;
     inj.fail_once = 1;
     memset(&cache, 0, sizeof(cache));
     cache.type = 1;
     cache.log = stderr;
     cache.errlog = stderr;
     cache.hashtable = coucal_new(0);
-    cache.zipOutput = selftest_open_failing_zip(path, &inj);
+    cache.zipOutput = selftest_open_failing_zip(path, &inj, HTS_TRUE);
     opt->state.exit_xh = 0;
 
     writefail_store(opt, &cache, "/blob.bin", body, body_len);
@@ -631,6 +651,19 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
               (int) cache.zipWriteFailed, opt->state.exit_xh);
       fail++;
     }
+    /* the rolled-back entry is truncated away, not merely rewound: #1402,
+       where a table with no truncate still reported the rollback as done */
+    if (inj.truncate == NULL) {
+      fprintf(stderr, "cache-writefail: skip: the table the cache opens with "
+                      "carries no truncate, so the rollback only rewound\n");
+      fail++;
+    } else if (inj.truncates != 1) {
+      fprintf(stderr,
+              "cache-writefail: skip: abandoned entry truncated %d time(s), "
+              "want 1\n",
+              inj.truncates);
+      fail++;
+    }
     writefail_store(opt, &cache, "/blob2.bin", body, 16);
     zipClose(cache.zipOutput, NULL);
     cache.zipOutput = NULL;
@@ -645,6 +678,57 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
     }
   }
 
+  /* a backend with no truncate rolls back by rewinding: the entry still drops
+     and the mirror still lives, and the incomplete rollback is warned about */
+  {
+    cache_back cache;
+    writefail_inject inj;
+    char extra[8192];
+    char rbody[64];
+    int n;
+
+    inj.budget = 4096;
+    inj.fail_errno = EIO;
+    inj.writes = 0;
+    inj.truncates = 0;
+    inj.fail_once = 1;
+    memset(&cache, 0, sizeof(cache));
+    cache.type = 1;
+    cache.log = stderr;
+    cache.errlog = stderr;
+    cache.hashtable = coucal_new(0);
+    cache.zipOutput = selftest_open_failing_zip(path, &inj, HTS_FALSE);
+    opt->state.exit_xh = 0;
+
+    writefail_store(opt, &cache, "/blob.bin", body, body_len);
+    if (cache.zipWriteFailed || opt->state.exit_xh != 0) {
+      fprintf(stderr,
+              "cache-writefail: notrunc: a rewind-only rollback aborted the "
+              "mirror (flagged=%d, exit_xh=%d)\n",
+              (int) cache.zipWriteFailed, opt->state.exit_xh);
+      fail++;
+    }
+    if (inj.truncates != 0) {
+      fprintf(stderr,
+              "cache-writefail: notrunc: %d truncate call(s) on a table that "
+              "has none\n",
+              inj.truncates);
+      fail++;
+    }
+    writefail_store(opt, &cache, "/blob2.bin", body, 16);
+    zipClose(cache.zipOutput, NULL);
+    cache.zipOutput = NULL;
+    n = writefail_read_entry(path, "http://example.com/blob2.bin", extra,
+                             sizeof(extra), rbody, sizeof(rbody));
+    if (n != 16 || memcmp(rbody, body, 16) != 0) {
+      fprintf(stderr,
+              "cache-writefail: notrunc: sibling entry lost after a rewind "
+              "(%d)\n",
+              n);
+      fail++;
+    }
+  }
+
   /* >2GB bodies: in-memory drops the entry, on-disk degrades to headers-only */
   {
     cache_back cache;
@@ -656,13 +740,14 @@ int cache_write_failure_selftest(httrackp *opt, const char *dir) {
     inj.budget = (size_t) -1; /* no injected failure */
     inj.fail_errno = 0;
     inj.writes = 0;
+    inj.truncates = 0;
     inj.fail_once = 0;
     memset(&cache, 0, sizeof(cache));
     cache.type = 1;
     cache.log = stderr;
     cache.errlog = stderr;
     cache.hashtable = coucal_new(0);
-    cache.zipOutput = selftest_open_failing_zip(path, &inj);
+    cache.zipOutput = selftest_open_failing_zip(path, &inj, HTS_TRUE);
     opt->state.exit_xh = 0;
 
     writefail_store_oversized(opt, &cache, "/bigmem.bin", 0 /* in-memory */);

src/htsselftest.c

@@ -4013,19 +4013,15 @@ static const char *const zip_abandon_kept[] = {"before.bin", "after1.bin",
                                                "after2.bin"};
 static const char zip_abandon_body[] = "zip-abandon kept member body";
 
-/* Build `path` with the kept members, opening a `doomed`-byte member after the
+/* Fill `zf` with the kept members, opening a `doomed`-byte member after the
    first one and abandoning it mid-write (0: never open it, the reference). The
-   doomed member is stored, so its body reaches the file byte for byte. */
-static int zip_abandon_build(const char *path, size_t doomed) {
-  char catbuff[CATBUFF_SIZE];
+   doomed member is stored, so its body reaches the file byte for byte. What the
+   abandon returned lands in *abandon_err. */
+static int zip_abandon_fill(zipFile zf, size_t doomed, int *abandon_err) {
   char chunk[4096];
   zip_fileinfo fi;
-  zipFile zf = hts_zipOpen_utf8(fconv(catbuff, sizeof(catbuff), path),
-                                APPEND_STATUS_CREATE);
   size_t i;
 
-  if (zf == NULL)
-    return -1;
   memset(&fi, 0, sizeof(fi));
   memset(chunk, 'Z', sizeof(chunk));
   for (i = 0; i < sizeof(zip_abandon_kept) / sizeof(zip_abandon_kept[0]); i++) {
@@ -4051,16 +4047,43 @@ static int zip_abandon_build(const char *path, size_t doomed) {
         goto fail;
       left -= n;
     }
-    if (zipAbandonFileInZip(zf) != ZIP_OK)
-      goto fail;
+    *abandon_err = zipAbandonFileInZip(zf);
   }
-  return zipClose(zf, NULL) == ZIP_OK ? 0 : -1;
+  return 0;
 
 fail:
-  zipClose(zf, NULL);
   return -1;
 }
 
+/* zip_abandon_fill() through a private filefunc table, `truncatable` telling
+   whether it carries a truncate entry: without one, as the Win32 tables were
+   before #1402, a rollback can only rewind. */
+static int zip_abandon_build_table(const char *path, size_t doomed,
+                                   hts_boolean truncatable, int *abandon_err) {
+  char catbuff[CATBUFF_SIZE];
+  zlib_filefunc64_def ff;
+  zipFile zf;
+  int ret;
+
+  hts_zip_filefunc64(&ff);
+  if (!truncatable)
+    ff.ztruncate64_file = NULL;
+  zf = zipOpen2_64(fconv(catbuff, sizeof(catbuff), path), APPEND_STATUS_CREATE,
+                   NULL, &ff);
+  if (zf == NULL)
+    return -1;
+  ret = zip_abandon_fill(zf, doomed, abandon_err);
+  return zipClose(zf, NULL) == ZIP_OK ? ret : -1;
+}
+
+static int zip_abandon_build(const char *path, size_t doomed) {
+  int abandon_err = ZIP_OK;
+
+  if (zip_abandon_build_table(path, doomed, HTS_TRUE, &abandon_err) != 0)
+    return -1;
+  return abandon_err == ZIP_OK ? 0 : -1;
+}
+
 /* Whole file into a malloct'd buffer: binary, not terminated. */
 static int zip_abandon_slurp(const char *path, char **out, size_t *len) {
   char catbuff[CATBUFF_SIZE];
@@ -4187,6 +4210,86 @@ static int st_zip_abandon(httrackp *opt, int argc, char **argv) {
   return fail;
 }
 
+/* A rollback that could only rewind must say so (#1402): reporting ZIP_OK for
+   it hands the caller an archive whose directory no reader finds, the very
+   damage the truncate is there to prevent. */
+static int st_zip_abandon_notrunc(httrackp *opt, int argc, char **argv) {
+  static const size_t doomed = 200000; /* a tail past unzip.c's 64KB backscan */
+  char refpath[HTS_URLMAXSIZE], path[HTS_URLMAXSIZE];
+  char *ref = NULL, *got = NULL;
+  size_t reflen = 0, gotlen = 0;
+  int abandon_err = ZIP_OK;
+  int fail = 0;
+  unzFile uf;
+
+  (void) opt;
+  if (argc < 1) {
+    fprintf(stderr, "zip-abandon-notrunc: needs a writable directory\n");
+    return 1;
+  }
+  fconcat(refpath, sizeof(refpath), argv[0], "zip-notrunc-ref.zip");
+  fconcat(path, sizeof(path), argv[0], "zip-notrunc.zip");
+
+  /* control: the table the cache opens with truncates, and reports ZIP_OK */
+  if (zip_abandon_build_table(refpath, doomed, HTS_TRUE, &abandon_err) != 0 ||
+      zip_abandon_slurp(refpath, &ref, &reflen) != 0 || reflen == 0) {
+    fprintf(stderr, "zip-abandon-notrunc: cannot build the reference\n");
+    freet(ref);
+    return 1;
+  }
+  freet(ref);
+  if (abandon_err != ZIP_OK) {
+    fprintf(stderr,
+            "zip-abandon-notrunc: a truncating backend returned %d, want %d\n",
+            abandon_err, ZIP_OK);
+    fail++;
+  }
+  /* a flushed-but-kept member shows up as more than a backscan of leftovers,
+     which is the size that matters, not the member's nominal one */
+  if (reflen > 65535) {
+    fprintf(stderr,
+            "zip-abandon-notrunc: the reference kept %d byte(s), so the "
+            "abandoned member was not truncated away\n",
+            (int) reflen);
+    fail++;
+  }
+
+  abandon_err = ZIP_OK;
+  if (zip_abandon_build_table(path, doomed, HTS_FALSE, &abandon_err) != 0 ||
+      zip_abandon_slurp(path, &got, &gotlen) != 0) {
+    fprintf(stderr, "zip-abandon-notrunc: cannot build '%s'\n", path);
+    freet(got);
+    return fail + 1;
+  }
+  freet(got);
+  if (abandon_err == ZIP_OK) {
+    fprintf(stderr, "zip-abandon-notrunc: a backend with no truncate reported "
+                    "the rollback as done\n");
+    fail++;
+  }
+  /* only what the member had flushed stays, and it takes more than a reader's
+     64KB backscan (unzip.c uMaxBack) to bury the directory behind it */
+  if (gotlen <= reflen + 65535) {
+    fprintf(stderr,
+            "zip-abandon-notrunc: %d byte(s) left over the reference's %d, too "
+            "few to outgrow a 64KB backscan\n",
+            (int) gotlen, (int) reflen);
+    fail++;
+  }
+  /* what the wrong return hides: the tail buries the directory written after */
+  uf = hts_unzOpen_utf8(path);
+  if (uf != NULL) {
+    fprintf(stderr,
+            "zip-abandon-notrunc: '%s' still opens, so the case proves "
+            "nothing\n",
+            path);
+    unzClose(uf);
+    fail++;
+  }
+  printf("zip-abandon-notrunc: %s\n", fail ? "FAIL" : "OK");
+  return fail;
+}
+
 static int st_cache_legacy(httrackp *opt, int argc, char **argv) {
   int err;
 
@@ -12434,6 +12537,9 @@ static const struct selftest_entry {
      st_zip_repair_shift},
     {"zip-abandon", "<dir>",
      "an abandoned member leaves the archive byte-identical", st_zip_abandon},
+    {"zip-abandon-notrunc", "<dir>",
+     "a rollback that could only rewind reports a failure",
+     st_zip_abandon_notrunc},
     {"dns", "", "DNS resolver/cache self-test", st_dns},
     {"dnstimeout", "", "a slow DNS resolve is bounded and holds no lock",
      st_dnstimeout},