Signed-shift UB in zip-repair local-header read

unzRepair parses the local file header of a damaged cache zip during
repair. READ_32 combined two int-typed READ_16 halves as
READ_16(adr) | (READ_16((adr)+2) << 16); when the high half has bit 15
set (>= 0x8000), shifting it left by 16 exceeds INT_MAX, which is signed
overflow. UBSan aborts on the CRC/size fields of a header whose high
16-bit word has that bit set. Repair runs on hostile input (a corrupt or
foreign new.zip). Cast the halves to uLong before the shift, matching the
sibling minizip readers in unzip.c and zip.c.

Closes #639

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>

src/htscoremain.c

@@ -2368,28 +2368,30 @@ static int hts_main_internal(int argc, char **argv, httrackp * opt) {
       opt->errlog = stderr;
     } else if (httrack_logmode >= 2) {
       // deux fichiers log
-      // path_log holds UTF-8 bytes (argv is UTF-8): the ANSI file calls would
-      // read them as the codepage and drop the logs into a mangled twin (#630).
-      structcheck_utf8(StringBuff(opt->path_log));
-      if (fexist_utf8(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                              StringBuff(opt->path_log), "hts-log.txt")))
-        UNLINK(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                       StringBuff(opt->path_log), "hts-log.txt"));
-      if (fexist_utf8(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                              StringBuff(opt->path_log), "hts-err.txt")))
-        UNLINK(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                       StringBuff(opt->path_log), "hts-err.txt"));
+      structcheck(StringBuff(opt->path_log));
+      if (fexist
+          (fconcat
+           (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-log.txt")))
+        remove(fconcat
+               (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-log.txt"));
+      if (fexist
+          (fconcat
+           (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-err.txt")))
+        remove(fconcat
+               (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-err.txt"));
 
       /* Check FS directory structure created */
-      structcheck_utf8(StringBuff(opt->path_log));
+      structcheck(StringBuff(opt->path_log));
 
-      opt->log = FOPEN(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                               StringBuff(opt->path_log), "hts-log.txt"),
-                       "w");
+      opt->log =
+        fopen(fconcat
+              (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-log.txt"),
+              "w");
       if (httrack_logmode == 2)
-        opt->errlog = FOPEN(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt),
-                                    StringBuff(opt->path_log), "hts-err.txt"),
-                            "w");
+        opt->errlog =
+          fopen(fconcat
+                (OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-err.txt"),
+                "w");
       else
         opt->errlog = opt->log;
       if (opt->log == NULL) {

src/htsname.c

@@ -1532,30 +1532,8 @@ int url_savename(lien_adrfilsave *const afs,
       // last segment
       wsave[j++] = '/';
 #define MAX_UTF8_SEQ_CHARS 4
-      {
-        // #623: the ".delayed" placeholder marker sits at the tail; cutting
-        // through it drops IS_DELAYED_EXT, so the file is never renamed to its
-        // final name. Reserve the trailing ".<id>.delayed" across the cut.
-        size_t markStart = wsaveLen;
-        if (IS_DELAYED_EXT(afs->save)) {
-          const size_t extDot = wsaveLen - strlen("." DELAYED_EXT);
-          size_t p = extDot; /* walk back over a dot-separated ".<hexid>" tag */
-
-          while (p > lastSeg && ((wsave[p - 1] >= '0' && wsave[p - 1] <= '9') ||
-                                 (wsave[p - 1] >= 'a' && wsave[p - 1] <= 'f')))
-            p--;
-          // keep the tag only if truly ".<hexid>.delayed"; else the bare marker
-          // (a wholly-hex base, e.g. a hashed #133 name, must not be absorbed)
-          markStart = (p > lastSeg && p < extDot && wsave[p - 1] == '.')
-                          ? p - 1
-                          : extDot;
-        }
-        // head, bounded so the marker still fits, then the marker itself
-        for (i = lastSeg; i < markStart && j + (wsaveLen - markStart) < maxLen;
-             i++)
-          wsave[j++] = wsave[i];
-        for (i = markStart; i < wsaveLen && j < maxLen; i++)
-          wsave[j++] = wsave[i];
+      for(i = lastSeg; wsave[i] != '\0' && j < maxLen; i++) {
+        wsave[j++] = wsave[i];
       }
       // terminating \0
       wsave[j++] = '\0';

src/htsselftest.c

@@ -2146,6 +2146,52 @@ static int st_cache_corrupt(httrackp *opt, int argc, char **argv) {
   return err;
 }
 
+/* Drives unzRepair over a damaged local file header whose CRC field's high
+   16-bit word has bit 15 set. Before the READ_32 fix that shifted an int and
+   overflowed, so UBSan aborts here; after it, repair recovers the one entry. */
+static int st_zip_repair_shift(httrackp *opt, int argc, char **argv) {
+  static const unsigned char zip[] = {
+      0x50, 0x4b, 0x03, 0x04, /* local file header signature */
+      0x14, 0x00,             /* version needed */
+      0x00, 0x00,             /* general purpose flag */
+      0x00, 0x00,             /* method */
+      0x00, 0x00,             /* time */
+      0x00, 0x00,             /* date */
+      0x00, 0x00, 0xe8, 0x8a, /* crc: high word 0x8ae8, bit 15 set */
+      0x00, 0x00, 0x00, 0x00, /* compressed size */
+      0x00, 0x00, 0x00, 0x00, /* uncompressed size */
+      0x01, 0x00,             /* filename length */
+      0x00, 0x00,             /* extra field length */
+      0x61                    /* filename "a" */
+  };
+  char in[HTS_URLMAXSIZE], out[HTS_URLMAXSIZE], tmp[HTS_URLMAXSIZE];
+  uLong nrec = 0, bytes = 0;
+  FILE *fp;
+  int err;
+
+  (void) opt;
+  if (argc < 1) {
+    fprintf(stderr, "zip-repair-shift: needs a directory\n");
+    return 1;
+  }
+  snprintf(in, sizeof(in), "%s/damaged.zip", argv[0]);
+  snprintf(out, sizeof(out), "%s/repair.zip", argv[0]);
+  snprintf(tmp, sizeof(tmp), "%s/repair.tmp", argv[0]);
+  fp = fopen(in, "wb");
+  if (fp == NULL || fwrite(zip, 1, sizeof(zip), fp) != sizeof(zip)) {
+    if (fp != NULL)
+      fclose(fp);
+    fprintf(stderr, "zip-repair-shift: cannot write %s\n", in);
+    return 1;
+  }
+  fclose(fp);
+  err = unzRepair(in, out, tmp, &nrec, &bytes);
+  printf("zip-repair-shift: %s (recovered %lu entr%s)\n",
+         (err == Z_OK && nrec == 1) ? "OK" : "FAIL", (unsigned long) nrec,
+         nrec == 1 ? "y" : "ies");
+  return (err == Z_OK && nrec == 1) ? 0 : 1;
+}
+
 static int st_cache_legacy(httrackp *opt, int argc, char **argv) {
   int err;
 
@@ -3253,6 +3299,9 @@ static const struct selftest_entry {
      st_cache_legacy},
     {"cache-corrupt", "<dir>", "cache read-side corruption self-test",
      st_cache_corrupt},
+    {"zip-repair-shift", "<dir>",
+     "cache zip-repair header read must not overflow a signed shift",
+     st_zip_repair_shift},
     {"dns", "", "DNS resolver/cache self-test", st_dns},
     {"dnstimeout", "", "a slow DNS resolve is bounded and holds no lock",
      st_dnstimeout},

src/minizip/mztools.c

@@ -14,7 +14,7 @@
 
 #define READ_8(adr)  ((unsigned char)*(adr))
 #define READ_16(adr) ( READ_8(adr) | (READ_8(adr+1) << 8) )
-#define READ_32(adr) ( READ_16(adr) | (READ_16((adr)+2) << 16) )
+#define READ_32(adr) ((uLong) READ_16(adr) | ((uLong) READ_16((adr) + 2) << 16))
 
 #define WRITE_8(buff, n) do { \
   *((unsigned char*)(buff)) = (unsigned char) ((n) & 0xff); \

tests/01_zlib-repair-shift.test

@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# Keep this POSIX-portable: the harness runs it via $(BASH), which is a plain
+# POSIX /bin/sh on some platforms (e.g. macOS), so avoid bashisms and GNU-only
+# tool flags despite the #!/bin/bash above.
+
+# unzRepair header read must not overflow a signed shift (-#test=zip-repair-shift
+# <dir>). A damaged local file header whose CRC high word has bit 15 set made
+# READ_32 shift an int past INT_MAX; UBSan aborts before the fix casts to uLong.
+
+set -eu
+
+dir=$(mktemp -d)
+trap 'rm -rf "$dir"' EXIT
+
+out=$(httrack -#test=zip-repair-shift "$dir")
+
+printf '%s\n' "$out" | grep -qx "zip-repair-shift: OK (recovered 1 entry)" || {
+    echo "expected 'zip-repair-shift: OK (recovered 1 entry)', got: $out" >&2
+    exit 1
+}

tests/67_engine-delayed-truncate.test

@@ -1,72 +0,0 @@
-#!/bin/bash
-#
-
-set -euo pipefail
-
-# #623: url_savename enforces the 236-char path ceiling by cutting the tail of
-# the last segment, where the mandatory ".delayed" placeholder marker lives. A
-# cut marker fails IS_DELAYED_EXT, so back_delayed_rename never renames the file
-# to its final name and the download is lost. The marker must survive the cut.
-
-# statuscode=302 status=-1 = a redirect answer still downloading: no type is
-# resolved, so the name gets a ".<id>.delayed" placeholder (see 01_engine-savename).
-CEIL=236
-
-httrack_bin=$(cd "$(dirname "$(command -v httrack)")" && pwd)/httrack
-scratch=$(mktemp -d)
-trap 'rm -rf "$scratch"' EXIT
-cd "$scratch"
-
-run() {
-    "$httrack_bin" -O /dev/null -#test=savename "$@" | sed -n 's/^savename: //p'
-}
-
-# A deep path ending in a long segment that overruns the ceiling.
-deep="/d1$(printf 'a%.0s' {1..40})/d2$(printf 'b%.0s' {1..40})"
-deep="$deep/d3$(printf 'c%.0s' {1..40})/d4$(printf 'd%.0s' {1..40})"
-long="$deep/$(printf 'z%.0s' {1..90})"
-
-out="$(run "$long" text/html statuscode=302 status=-1)"
-case "$out" in
-*.delayed) ;;
-*)
-    echo "FAIL: delayed marker cut by truncation: '$out'"
-    exit 1
-    ;;
-esac
-test "${#out}" -le "$CEIL" || {
-    echo "FAIL: truncated name ${#out} > $CEIL ceiling: '$out'"
-    exit 1
-}
-
-# #133-style hashed name: an all-hex last segment must keep the marker too (the
-# ".<id>." tag is not mistaken for part of the hash).
-hexseg=$(printf 'a1b2c3d4e5f60718%.0s' {1..8})
-hexpath="/d1$(printf 'x%.0s' {1..30})/d2$(printf 'y%.0s' {1..30})/$hexseg"
-out="$(run "$hexpath" text/html statuscode=302 status=-1)"
-case "$out" in
-*.delayed) ;;
-*)
-    echo "FAIL: hashed-name marker cut by truncation: '$out'"
-    exit 1
-    ;;
-esac
-test "${#out}" -le "$CEIL" || {
-    echo "FAIL: hashed name ${#out} > $CEIL ceiling: '$out'"
-    exit 1
-}
-
-# A non-delayed name of the same shape still truncates, with no marker to keep.
-out="$(run "$long.html" text/html)"
-test "${#out}" -le "$CEIL" || {
-    echo "FAIL: non-delayed name ${#out} > $CEIL ceiling: '$out'"
-    exit 1
-}
-case "$out" in
-*.delayed)
-    echo "FAIL: non-delayed name grew a .delayed marker: '$out'"
-    exit 1
-    ;;
-esac
-
-echo "delayed-truncate OK"

tests/69_local-intl-logdir.test

@@ -1,15 +0,0 @@
-#!/bin/bash
-#
-# A single non-ASCII -O sets both path_html and path_log to "café" (#630). The
-# logs (hts-log.txt/hts-err.txt) must land there, not in an ANSI-mangled twin:
-# on Windows path_log holds UTF-8 bytes the raw file calls read as the codepage.
-# The --errors/--files/--log-found audits all grep logroot=café/hts-log.txt, so
-# a log written to the twin fails them. POSIX has no twin, so this bites on the
-# Windows CI leg (like test 64). The cache twin is a separate, larger fix.
-
-: "${top_srcdir:=..}"
-
-bash "$top_srcdir/tests/local-crawl.sh" --outdir-intl 'café' --errors 0 --files 5 \
-    --found 'simple/basic.html' \
-    --log-found 'mirror complete in' \
-    httrack 'BASEURL/simple/basic.html'

tests/Makefile.am

@@ -84,6 +84,7 @@ TESTS = \
 	01_zlib-cache-legacy.test \
 	01_zlib-cache-golden.test \
 	01_zlib-cache-writefail.test \
+	01_zlib-repair-shift.test \
 	01_zlib-savename-cached.test \
 	02_manpage-regen.test \
 	02_update-cache.test \
@@ -147,8 +148,6 @@ TESTS = \
 	63_webhttrack-home.test \
 	64_local-intl-outdir.test \
 	65_port-siblings.test \
-	66_engine-port80-strip.test \
-	67_engine-delayed-truncate.test \
-	69_local-intl-logdir.test
+	66_engine-port80-strip.test
 
 CLEANFILES = check-network_sh.cache

tests/local-crawl.sh

@@ -48,7 +48,6 @@ key="${testdir}/server.key"
 tls=
 verbose=
 html_subdir=
-outdir_intl=
 rerun=
 rerun_args=
 rerun_dead=
@@ -141,13 +140,6 @@ while test "$pos" -lt "$nargs"; do
         pos=$((pos + 1))
         html_subdir="${args[$pos]}"
         ;;
-    --outdir-intl)
-        # Single non-ASCII -O "$out/NAME": path_html AND path_log are NAME, so
-        # the logs (and the harness reads of them) go through the non-ASCII path
-        # (#630). Distinct from --html-subdir, which keeps path_log ASCII.
-        pos=$((pos + 1))
-        outdir_intl="${args[$pos]}"
-        ;;
     --errors | --errors-content | --files)
         audit+=("${args[$pos]}" "${args[$((pos + 1))]}")
         pos=$((pos + 1))
@@ -224,18 +216,13 @@ test -n "$ver" || die "could not run httrack"
 out="${tmpdir}/crawl"
 mkdir "$out" || die "could not create $out"
 # path_html holds the mirror + index; path_log holds hts-cache/hts-log.txt.
-# Default: both are "$out". --html-subdir moves path_html to "$out/NAME" while
-# path_log (logroot) stays "$out"; --outdir-intl moves both to "$out/NAME".
+# Default: both are "$out". With --html-subdir, path_html becomes "$out/NAME"
+# (the mirror root the audits inspect) while path_log stays "$out".
 mirrorroot="$out"
-logroot="$out"
 odir="$out"
 if test -n "$html_subdir"; then
     mirrorroot="${out}/${html_subdir}"
     odir="${mirrorroot},${out}"
-elif test -n "$outdir_intl"; then
-    mirrorroot="${out}/${outdir_intl}"
-    logroot="$mirrorroot"
-    odir="$mirrorroot"
 fi
 # Localhost is fast; disable the rate/bandwidth safety limits but keep a
 # max-time backstop so a hang cannot wedge the suite.
@@ -254,7 +241,7 @@ test "$crawlres" -eq 0 || ! result "httrack exited $crawlres" || {
     exit 1
 }
 result "OK"
-grep -iE "^[0-9:]*[[:space:]]Error:" "${logroot}/hts-log.txt" >&2
+grep -iE "^[0-9:]*[[:space:]]Error:" "${out}/hts-log.txt" >&2
 
 # --- optional second pass: re-mirror into the same dir (cache/update path) ----
 if test -n "$rerun"; then
@@ -273,7 +260,7 @@ if test -n "$rerun"; then
     # The update summary reports "files updated"; a fresh crawl never does. Assert
     # it so a regression that bypasses the cache (re-crawls fresh) can't pass.
     info "checking update used the cache"
-    if grep -aqE "mirror complete in .*files updated" "${logroot}/hts-log.txt"; then
+    if grep -aqE "mirror complete in .*files updated" "${out}/hts-log.txt"; then
         result "OK"
     else
         result "update pass did not report cache activity"
@@ -305,7 +292,7 @@ if test -n "$rerun_dead"; then
     zip="${out}/hts-cache/new.zip"
     test -s "$zip" || die "no cache was written by the first pass"
     cp "$zip" "${tmpdir}/cache-before.zip"
-    cp "${logroot}/hts-log.txt" "${tmpdir}/log-before.txt"
+    cp "${out}/hts-log.txt" "${tmpdir}/log-before.txt"
     stop_server "$serverpid"
     serverpid=
     info "re-running httrack against the stopped server"
@@ -318,7 +305,7 @@ if test -n "$rerun_dead"; then
     # The dead pass must have gone through the no-data rollback, not bailed out
     # before the mirror loop (which would leave the cache trivially untouched).
     info "checking the dead pass hit the rollback"
-    if grep -aq "No data seems to have been transferred" "${logroot}/hts-log.txt"; then
+    if grep -aq "No data seems to have been transferred" "${out}/hts-log.txt"; then
         result "OK"
     else
         result "rollback notice not found in hts-log.txt"
@@ -333,7 +320,7 @@ if test -n "$rerun_dead"; then
         exit 1
     fi
     # Audits below describe the healthy crawl, not the dead pass.
-    cp "${tmpdir}/log-before.txt" "${logroot}/hts-log.txt"
+    cp "${tmpdir}/log-before.txt" "${out}/hts-log.txt"
 fi
 
 # --- discover the single host root (127.0.0.1_<port> or 127.0.0.1) -----------
@@ -363,19 +350,19 @@ while test "$i" -lt "${#audit[@]}"; do
     --errors)
         i=$((i + 1))
         assert_equals "checking errors" "${audit[$i]}" \
-            "$(grep -iEc "^[0-9:]*[[:space:]]Error:" "${logroot}/hts-log.txt")"
+            "$(grep -iEc "^[0-9:]*[[:space:]]Error:" "${out}/hts-log.txt")"
         ;;
     --errors-content)
         i=$((i + 1))
-        total=$(grep -icE "^[0-9:]*[[:space:]]Error:" "${logroot}/hts-log.txt")
+        total=$(grep -icE "^[0-9:]*[[:space:]]Error:" "${out}/hts-log.txt")
         # transient network failures (statuscode -2..-6) flake on busy loopback;
         # the code parens are followed by " at link" or " after N retries at link"
-        transient=$(grep -cE '\(-[2-6]\) (at link|after )' "${logroot}/hts-log.txt" || true)
+        transient=$(grep -cE '\(-[2-6]\) (at link|after )' "${out}/hts-log.txt" || true)
         assert_equals "checking content errors" "${audit[$i]}" "$((total - transient))"
         ;;
     --files)
         i=$((i + 1))
-        nFiles=$(grep -E "^HTTrack Website Copier/[^ ]* mirror complete in " "${logroot}/hts-log.txt" |
+        nFiles=$(grep -E "^HTTrack Website Copier/[^ ]* mirror complete in " "${out}/hts-log.txt" |
             sed -e 's/.*[[:space:]]\([^ ]*\)[[:space:]]files written.*/\1/g')
         assert_equals "checking files" "${audit[$i]}" "$nFiles"
         ;;
@@ -406,7 +393,7 @@ while test "$i" -lt "${#audit[@]}"; do
     --log-found)
         i=$((i + 1))
         info "checking log matches ${audit[$i]}"
-        if grep -aqE "${audit[$i]}" "${logroot}/hts-log.txt"; then result "OK"; else
+        if grep -aqE "${audit[$i]}" "${out}/hts-log.txt"; then result "OK"; else
             result "not in log"
             exit 1
         fi
@@ -414,7 +401,7 @@ while test "$i" -lt "${#audit[@]}"; do
     --log-not-found)
         i=$((i + 1))
         info "checking log lacks ${audit[$i]}"
-        if grep -aqE "${audit[$i]}" "${logroot}/hts-log.txt"; then
+        if grep -aqE "${audit[$i]}" "${out}/hts-log.txt"; then
             result "present in log"
             exit 1
         else result "OK"; fi