src/pefile: stricter reloc checks; cleanups

.github/workflows/ci.yml

@@ -12,8 +12,8 @@ env:
   CMAKE_REQUIRED_QUIET: OFF
   DEBIAN_FRONTEND: noninteractive
   UPX_CMAKE_BUILD_FLAGS: --verbose
-  # 2023-10-21
+  # 2023-10-22
-  ZIG_DIST_VERSION: 0.12.0-dev.1137+fbbccc9d5
+  ZIG_DIST_VERSION: 0.12.0-dev.1153+45d7dfa83
 
 jobs:
   job-rebuild-and-verify-stubs:

.github/workflows/weekly-ci-cc-zigcc.yml

@@ -10,8 +10,8 @@ on:
 env:
   CMAKE_REQUIRED_QUIET: OFF
   DEBIAN_FRONTEND: noninteractive
-  # 2023-10-21
+  # 2023-10-22
-  ZIG_DIST_VERSION: 0.12.0-dev.1137+fbbccc9d5
+  ZIG_DIST_VERSION: 0.12.0-dev.1153+45d7dfa83
 
 jobs:
   job-linux-zigcc: # uses cmake + make

CMakeLists.txt

@@ -168,8 +168,8 @@ endif()
 #***********************************************************************
 
 include(CheckCCompilerFlag)
-include(CheckIncludeFile)
 include(CheckFunctionExists)
+include(CheckIncludeFile)
 include(CheckStructHasMember)
 include(CheckSymbolExists)
 
@@ -181,11 +181,11 @@ if(NOT DEFINED HAVE_UTIMENSAT)
     if(HAVE_UTIMENSAT_FUNCTION__)
         check_symbol_exists(utimensat "sys/types.h;fcntl.h;sys/stat.h" HAVE_UTIMENSAT_SYMBOL__)
         if(HAVE_UTIMENSAT_SYMBOL__)
-            CHECK_STRUCT_HAS_MEMBER("struct stat" st_mtim.tv_nsec "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC)
+            CHECK_STRUCT_HAS_MEMBER("struct stat" "st_mtim.tv_nsec" "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC)
-            if (NOT HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC)
+            if(NOT HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC)
-                CHECK_STRUCT_HAS_MEMBER("struct stat" st_mtimespec.tv_nsec "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
+                CHECK_STRUCT_HAS_MEMBER("struct stat" "st_mtimespec.tv_nsec" "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
             endif()
-            if (HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC OR HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
+            if(HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC OR HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
                 set(HAVE_UTIMENSAT 1)
             endif()
         endif()
@@ -452,7 +452,7 @@ if(NOT UPX_CONFIG_DISABLE_ZSTD)
 endif()
 if(HAVE_UTIMENSAT)
     target_compile_definitions(${t} PRIVATE USE_UTIMENSAT=1)
-    if (HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
+    if(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)
         target_compile_definitions(${t} PRIVATE HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC=1)
     endif()
 endif()

NEWS

@@ -4,6 +4,7 @@ User visible changes for UPX
 
 Changes in 4.2.0 (XX XXX 2023):
   * disable macOS support until we fix compatibility with macOS 13+
+  * win32/pe and win64/pe: stricter relocation checks; please report regressions
   * new option '--link' to preserve hard-links (Unix only; use with care)
   * add support for NO_COLOR env var; see https://no-color.org/
   * bug fixes - see https://github.com/upx/upx/milestone/13

src/pefile.cpp

@@ -295,7 +295,7 @@ struct FixDeleter { // don't leak memory on exceptions
 };
 } // namespace
 
-static constexpr unsigned RELOC_BUF_OFFSET = 64 * 1024;
+static constexpr unsigned RELOC_INPLACE_OFFSET = 64 * 1024;
 
 PeFile::Reloc::~Reloc() noexcept {
     COMPILE_TIME_ASSERT(sizeof(BaseReloc) == 8)
@@ -318,8 +318,6 @@ PeFile::Reloc::Reloc(byte *s, unsigned si) : start(s), start_size_in_bytes(si) {
         if (x == 0 || x == BRS)               // ignore strange empty relocs
             return;
     }
-    if (si < BRS + sizeof(LE16)) // 10
-        throwCantPack("bad reloc size %d", si);
     // fill counts
     unsigned pos, type;
     while (next(pos, type))
@@ -327,7 +325,7 @@ PeFile::Reloc::Reloc(byte *s, unsigned si) : start(s), start_size_in_bytes(si) {
 }
 
 PeFile::Reloc::Reloc(unsigned relocnum) {
-    start_size_in_bytes = mem_size(4, relocnum, RELOC_BUF_OFFSET, 8192);
+    start_size_in_bytes = mem_size(4, relocnum, RELOC_INPLACE_OFFSET, 8192);
     start = new byte[start_size_in_bytes]; // => transfer to oxrelocs[] in finish()
     start_did_alloc = true;
     initSpans();
@@ -341,13 +339,22 @@ void PeFile::Reloc::initSpans() {
     rel1 = nullptr;
 }
 
-void PeFile::Reloc::advanceBaseRelocPos(void *p) {
+void PeFile::Reloc::setBaseRelocPos(void *p, bool check_size_of_block) { // set rel and rel1
     unsigned off = ptr_udiff_bytes(p, start);
+    assert((off & 3) == 0);
+    rel = (BaseReloc *) p;
     if (!start_did_alloc && off == start_size_in_bytes) { // final entry
-        rel = (BaseReloc *) p;
         rel1 = nullptr;
     } else {
-        rel = (BaseReloc *) p;
+        if (off + 10 > start_size_in_bytes)
+            throwCantPack("bad relocs");
+        if (check_size_of_block && !opt->force) {
+            const unsigned s = rel->size_of_block;
+            if (s < sizeof(BaseReloc) + sizeof(LE16)) // == 10
+                throwCantPack("bad reloc size_of_block %u (try --force)", s);
+            if ((s & 3) != 0)
+                throwCantPack("unaligned reloc size_of_block %u (try --force)", s);
+        }
         rel1 = (LE16 *) ((byte *) p + sizeof(BaseReloc));
     }
 }
@@ -357,7 +364,7 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) {
     unsigned pos, type;
     do {
         if (rel == nullptr)
-            advanceBaseRelocPos(start);
+            setBaseRelocPos(start, true);
         if (ptr_udiff_bytes(rel, start) >= start_size_in_bytes) {
             rel = nullptr;  // rewind
             rel1 = nullptr; // rewind
@@ -367,7 +374,7 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) {
         type = *rel1++ >> 12;
         NO_printf("%x %d\n", pos, type);
         if (ptr_udiff_bytes(rel1, rel) >= rel->size_of_block)
-            advanceBaseRelocPos(raw_bytes(rel1, 0));
+            setBaseRelocPos(raw_bytes(rel1, 0), true);
     } while (type == 0);
     result_pos = pos;
     result_type = type;
@@ -376,53 +383,66 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) {
 
 void PeFile::Reloc::add(unsigned pos, unsigned type) {
     assert(start_did_alloc);
-    set_le32(start_buf + (RELOC_BUF_OFFSET + 4 * counts[0]), (pos << 4) + type);
+    set_le32(start_buf + (RELOC_INPLACE_OFFSET + 4 * counts[0]), (pos << 4) + type);
     counts[0] += 1;
 }
 
 void PeFile::Reloc::finish(byte *(&result_ptr), unsigned &result_size) {
     assert(start_did_alloc);
-    // sentinel to force final advanceBaseRelocPos()
+    upx_qsort(raw_index_bytes(start_buf, RELOC_INPLACE_OFFSET, 4 * counts[0]), counts[0], 4,
-    set_le32(start_buf + (RELOC_BUF_OFFSET + 4 * counts[0]), 0xfff00000);
-    counts[0] += 1;
-    upx_qsort(raw_index_bytes(start_buf, RELOC_BUF_OFFSET, 4 * counts[0]), counts[0], 4,
               le32_compare);
 
+    auto align_size_of_block = [](SPAN_S(BaseReloc) xrel) {
+        assert(xrel->size_of_block >= 10);
+        auto end = SPAN_TYPE_CAST(byte, xrel) + xrel->size_of_block;
+        while ((xrel->size_of_block & 3) != 0) {
+            *end++ = 0; // clear byte
+            xrel->size_of_block += 1;
+        }
+    };
+
     rel = nullptr;
     rel1 = nullptr;
-    unsigned prev = 0xffffffff;
+    unsigned prev = 0;
     for (unsigned ic = 0; ic < counts[0]; ic++) {
-        const unsigned pos = get_le32(start_buf + (RELOC_BUF_OFFSET + 4 * ic));
+        const auto pos_ptr = start_buf + (RELOC_INPLACE_OFFSET + 4 * ic);
-        if (rel == (BaseReloc *) (void *) start)
+        if (rel1 != nullptr && ptr_diff_bytes(rel1, pos_ptr) >= 0) {
-            if (ptr_udiff_bytes(rel1, rel) > RELOC_BUF_OFFSET - sizeof(*rel1))
+            // info: if this is indeed a valid file we must increase RELOC_INPLACE_OFFSET
-                throwCantPack("too many relocs");
+            throwCantPack("too many relocs");
-        if (ic == 0) {
+        }
+        const unsigned pos = get_le32(pos_ptr);
+        if (ic > 0 && get_le32(pos_ptr - 4) == pos) // XXX: should we check for duplicates?
+            if (!opt->force)
+                throwCantPack("duplicate relocs (try --force)");
+        if (ic == 0 || (pos ^ prev) >= 0x10000) {
             prev = pos;
-            advanceBaseRelocPos(start);
+            if (ic == 0)
+                setBaseRelocPos(start, false);
+            else {
+                align_size_of_block(rel);
+                setBaseRelocPos((byte *) raw_bytes(rel, rel->size_of_block) + rel->size_of_block,
+                                false);
+            }
             rel->pagestart = (pos >> 4) & ~0xfff;
-            rel->size_of_block = unsigned(-1); // to be filled later
+            rel->size_of_block = 8;
-        } else if ((pos ^ prev) >= 0x10000) {
-            prev = pos;
-            *rel1 = 0;                                                     // clear align-up memory
-            rel->size_of_block = ALIGN_UP(ptr_udiff_bytes(rel1, rel), 4u); // <= FILL
-            advanceBaseRelocPos((char *) raw_bytes(rel, rel->size_of_block) + rel->size_of_block);
-            rel->pagestart = (pos >> 4) & ~0xfff;
-            rel->size_of_block = unsigned(-1); // to be filled later
         }
         *rel1++ = (pos << 12) + ((pos >> 4) & 0xfff);
+        rel->size_of_block += 2;
+    }
+    result_size = 0; // result_size can be 0 in 64-bit mode
+    if (counts[0] != 0) {
+        align_size_of_block(rel);
+        result_size = ptr_udiff_bytes(rel, start) + rel->size_of_block;
     }
-    assert(ptr_udiff_bytes(rel1, rel) == 10); // sentinel
-    result_size = ptr_udiff_bytes(rel, start);
     assert((result_size & 3) == 0);
-    // assert(result_size > 0); // result_size can be 0 in 64-bit mode
     // transfer ownership
     assert(start_did_alloc);
     result_ptr = start;
-    start = nullptr;
     start_did_alloc = false;
-    SPAN_INVALIDATE(start_buf); // safety
+    ptr_invalidate_and_poison(start); // safety
-    SPAN_INVALIDATE(rel);       // safety
+    SPAN_INVALIDATE(start_buf);       // safety
-    SPAN_INVALIDATE(rel1);      // safety
+    SPAN_INVALIDATE(rel);             // safety
+    SPAN_INVALIDATE(rel1);            // safety
 }
 
 void PeFile::processRelocs(Reloc *rel) // pass2

src/pefile.h

@@ -396,7 +396,7 @@ protected:
         };
         SPAN_0(BaseReloc) rel = nullptr;
         SPAN_0(LE16) rel1 = nullptr;
-        void advanceBaseRelocPos(void *p);
+        void setBaseRelocPos(void *p, bool check_size_of_block); // set rel and rel1
 
         unsigned counts[16] = {};
 

src/stub/Makefile

@@ -4,7 +4,7 @@
 #
 
 # NOTE: see misc/podman/rebuild-stubs/20-image-run-shell.sh
-#   how to rebuild the UPX stubs
+#   how to rebuild the UPX stubs from source code
 
 MAKEFLAGS += -rR
 .NOTPARALLEL:

src/stub/scripts/bin2h.py

@@ -349,7 +349,7 @@ def main(argv):
     if opts.mode == "c":
         if opts.verbose >= 0:
             w_header_c(w, ifile, ofile, len(idata))
-        w("/* clang" + "-format off */\n\n")
+        w("/* clang" + "-format" + " off */\n\n")
     for i in range(len(mdata)):
         write_stub(w, mdata_odata[mdata[i]], i, mdata)
     if ofp:

src/util/util.h

@@ -156,8 +156,9 @@ void upx_std_stable_sort(void *array, size_t n, upx_compare_func_t compare);
 
 // #define UPX_CONFIG_USE_STABLE_SORT 1
 #if UPX_CONFIG_USE_STABLE_SORT
-// use std::stable_sort(); requires that "element_size" is constexpr!
+// use std::stable_sort(); NOTE: requires that "element_size" is constexpr!
-#define upx_qsort(a, n, element_size, compare) upx_std_stable_sort<(element_size)>(a, n, compare)
+#define upx_qsort(a, n, element_size, compare)                                                     \
+    upx_std_stable_sort<(element_size)>((a), (n), (compare))
 #else
 // use libc qsort(); good enough for our use cases
 #define upx_qsort qsort

src/util/xspan_fwd.h

@@ -54,38 +54,6 @@ template <class T, class U>
 inline typename std::enable_if<std::is_integral<U>::value, void *>::type operator+(U, const C<T> &)
     XSPAN_DELETED_FUNCTION;
 
-#if 0 // already handled by member functions
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C<T> &a, const U *b) { return a.raw_bytes(0) == b; }
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C<T> &a, const C<U> &b) {
-    return a.raw_bytes(0) == b.raw_bytes(0);
-}
-#ifdef D
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C<T> &a, const D<U> &b) {
-    return a.raw_bytes(0) == b.raw_bytes(0);
-}
-#endif
-#ifdef E
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C<T> &a, const E<U> &b) {
-    return a.raw_bytes(0) == b.raw_bytes(0);
-}
-#endif
-
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C<T> &a, const U *b) { return a.raw_bytes(0) != b; }
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C<T> &a, const C<U> &b) {
-    return a.raw_bytes(0) != b.raw_bytes(0);
-}
-#ifdef D
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C<T> &a, const D<U> &b) {
-    return a.raw_bytes(0) != b.raw_bytes(0);
-}
-#endif
-#ifdef E
-XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C<T> &a, const E<U> &b) {
-    return a.raw_bytes(0) != b.raw_bytes(0);
-}
-#endif
-#endif // if 0 // already handled by member functions
-
 #endif // XSPAN_FWD_C_IS_MEMBUFFER
 
 /*************************************************************************

src/version.h

@@ -2,6 +2,6 @@
 #define UPX_VERSION_HEX      0x040200 /* 04.02.00 */
 #define UPX_VERSION_STRING   "4.2.0"
 #define UPX_VERSION_STRING4  "4.20"
-#define UPX_VERSION_DATE     "Oct 5th 2023"
+#define UPX_VERSION_DATE     "Oct 22nd 2023"
-#define UPX_VERSION_DATE_ISO "2023-10-05"
+#define UPX_VERSION_DATE_ISO "2023-10-22"
 #define UPX_VERSION_YEAR     "2023"

src/work.cpp

@@ -108,9 +108,11 @@ static void copy_file_attributes(const struct stat *st, const char *oname, bool
         struct timespec times[2];
         memset(&times[0], 0, sizeof(times));
 #if HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC
+        // macOS
         times[0] = st->st_atimespec;
         times[1] = st->st_mtimespec;
 #else
+        // POSIX.1-2008
         times[0] = st->st_atim;
         times[1] = st->st_mtim;
 #endif
@@ -118,7 +120,7 @@ static void copy_file_attributes(const struct stat *st, const char *oname, bool
         IGNORE_ERROR(r);
 #elif USE_UTIME
         struct utimbuf u;
-        memset(&u, 0, sizeof(u));
+        mem_clear(&u);
         u.actime = st->st_atime;
         u.modtime = st->st_mtime;
         int r = utime(oname, &u);
@@ -235,7 +237,7 @@ void do_one_file(const char *const iname, char *const oname) may_throw {
             if (opt->output_name) {
                 strcpy(tname, opt->output_name);
                 if ((opt->force_overwrite || opt->force >= 2) && !preserve_link)
-                    (void) FileBase::unlink_noexcept(tname);
+                    (void) FileBase::unlink_noexcept(tname); // IGNORE_ERROR
             } else {
                 if (st.st_nlink < 2)
                     preserve_link = false; // not needed
@@ -253,8 +255,8 @@ void do_one_file(const char *const iname, char *const oname) may_throw {
                     preserve_link = ost.st_nlink >= 2;
                 } else if (r == 0 && S_ISLNK(ost.st_mode)) {
                     // output_name is a symlink (valid or dangling)
-                    (void) FileBase::unlink_noexcept(tname);
+                    (void) FileBase::unlink_noexcept(tname); // IGNORE_ERROR
-                    preserve_link = false; // not needed
+                    preserve_link = false;                   // not needed
                 } else {
                     preserve_link = false; // not needed
                 }
@@ -361,8 +363,8 @@ void do_one_file(const char *const iname, char *const oname) may_throw {
 
 static void unlink_ofile(char *oname) noexcept {
     if (oname && oname[0]) {
-        (void) FileBase::unlink_noexcept(oname);
+        (void) FileBase::unlink_noexcept(oname); // IGNORE_ERROR
-        oname[0] = 0; // done with oname
+        oname[0] = 0;                            // done with oname
     }
 }