[PATCH rebase] Fix handling of unset PE checksums

[PATCH rebase] Fix handling of unset PE checksums

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

Tests with non-Cygwin executables show that other toolchains (LLVM LLD, 
MS link without /RELEASE option) leave the PE checksum field as zero. 
Peflags+rebase 4.6.6 still work correctly then, but rebase prints a 
misleading warning 'Checksum update failed' in this case. With the 
attached patch, a zero checksum is left as is and no message is printed.

This currently does not affect typical Cygwin use cases, so there is IMO 
no urgent need for a new package.

BTW, there was possibly no announcement for the rebase 4.6.6 package.

-- 
Regards,
Christian


[-- Attachment #2: 0001-Fix-handling-of-unset-PE-checksums.patch --]
[-- Type: text/plain, Size: 6100 bytes --]

From 855be8d72a1f96d0a3cfcb0a97fbe12a9a912748 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Mon, 9 Oct 2023 09:56:05 +0200
Subject: [PATCH] Fix handling of unset PE checksums

Leave unset (zero) checksums as is unless checksum update is
explicitly requested.  Remove misleading peflags warning
'Checksum update failed'.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 imagehelper/objectfile.cc |  9 ++++++--
 peflags.c                 | 45 ++++++++++++++++++++++-----------------
 rebase.c                  |  7 +++---
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/imagehelper/objectfile.cc b/imagehelper/objectfile.cc
index 651fcce..ce4b444 100755
--- a/imagehelper/objectfile.cc
+++ b/imagehelper/objectfile.cc
@@ -497,13 +497,18 @@ BOOL LinkedObjectFile::updateCheckSum()
     p_checksum = &getNTHeader64 ()->OptionalHeader.CheckSum;
   else
     p_checksum = &getNTHeader32 ()->OptionalHeader.CheckSum;
+  uint32_t checksum = *p_checksum;
+
+  // Leave unset checksum as is
+  if (!checksum)
+    return TRUE;
 
   // Add changes from NT header to checksum updated by relocs
   uint16_t checksum16;
   if (relocs)
     checksum16 = relocs->getCheckSum16();
   else
-    checksum16 = pe32_checksum_update_begin(*p_checksum, FileSize);
+    checksum16 = pe32_checksum_update_begin(checksum, FileSize);
 
   if (is64bit ())
     checksum16 = pe32_checksum_update_add(checksum16, &old_ntheader64,
@@ -514,7 +519,7 @@ BOOL LinkedObjectFile::updateCheckSum()
 					 getNTHeader32 (),
 					 sizeof(old_ntheader32));
 
-  uint32_t checksum = pe32_checksum_update_end(checksum16, FileSize);
+  checksum = pe32_checksum_update_end(checksum16, FileSize);
   if (!checksum)
     return FALSE;
 
diff --git a/peflags.c b/peflags.c
index 917088b..14ea78d 100644
--- a/peflags.c
+++ b/peflags.c
@@ -508,7 +508,7 @@ do_mark (const char *pathname)
 
     }
 
-  /* Update the checksum. */
+  /* Update the checksum if set. */
   changed = FALSE;
   {
     const void *p_oldheader, *p_newheader;
@@ -529,20 +529,24 @@ do_mark (const char *pathname)
 	p_checksum = &pep->ntheader32->OptionalHeader.CheckSum;
       }
 
-    hdr_checksum = pe32_checksum_update_end(
-      pe32_checksum_update_add(
-	pe32_checksum_update_begin(*p_checksum, pep->filesize),
-	p_oldheader, p_newheader, headersize
-      ),
-      pep->filesize
-    );
-
-    if (!hdr_checksum)
-      fprintf(stderr, "%s: Checksum update failed\n", pathname);
-    else if (*p_checksum != hdr_checksum)
+    hdr_checksum = *p_checksum;
+    if (hdr_checksum)
       {
-	changed = TRUE;
-	*p_checksum = hdr_checksum;
+	ULONG new_hdr_checksum = pe32_checksum_update_end (
+	  pe32_checksum_update_add (
+	    pe32_checksum_update_begin (hdr_checksum, pep->filesize),
+	    p_oldheader, p_newheader, headersize
+	  ),
+	  pep->filesize
+	);
+
+	/* Update the checksum or set it to 0 if the old value is invalid. */
+	if (new_hdr_checksum != hdr_checksum
+	    && (mark_any || handle_any_sizeof == DO_WRITE))
+	  {
+	    changed = TRUE;
+	    *p_checksum = hdr_checksum = new_hdr_checksum;
+	  }
       }
   }
 
@@ -650,7 +654,7 @@ static void
 do_checksum (const char *pathname, int indent, int changed, ULONG hdr_checksum)
 {
   const char name[] = "PE file checksum";
-  if (checksum_option > 1)
+  if (checksum_option > 1 && (checksum_option > 2 || hdr_checksum))
     {
       int fd;
       unsigned new_checksum, old_checksum;
@@ -684,10 +688,11 @@ do_checksum (const char *pathname, int indent, int changed, ULONG hdr_checksum)
 	  printf ("%*s%-24s: 0x%08x (*needs update to 0x%08x*)\n", indent, "", name,
 		  old_checksum, new_checksum);
     }
-  else /* (checksum_option == 1) */
+  else /* (checksum_option == 1 || (checksum_option == 2 && !hdr_checksum)) */
     {
-      printf ("%*s%-24s: 0x%08x (%schanged, not verified)\n", indent, "", name,
-	      hdr_checksum, (changed ? "" : "un"));
+      printf ("%*s%-24s: 0x%08x (%schanged%s)\n", indent, "", name,
+	      hdr_checksum, (changed ? "" : "un"),
+	      (hdr_checksum ? ", not verified" : ""));
     }
 }
 
@@ -1265,8 +1270,8 @@ help (FILE *f)
 "                              separately in another file.\n"
 "  -k, --checksum     [BOOL]   Displays (no argument), verifies (false) or\n"
 "                              always fixes (true) the file checksum in the PE\n"
-"                              header.  By default, the checksum is updated\n"
-"                              without reading the whole file under the\n"
+"                              header.  By default, a set (nonzero) checksum is\n"
+"                              updated without reading the whole file under the\n"
 "                              assumption that the original checksum was\n"
 "                              correct.\n"
 "  -x, --stack-reserve [NUM]   Reserved stack size of the process in bytes.\n"
diff --git a/rebase.c b/rebase.c
index 6a531d0..3325c5e 100644
--- a/rebase.c
+++ b/rebase.c
@@ -1676,9 +1676,10 @@ Rebase PE files, usually DLLs, to a specified address or address range.\n\
   -c, --checksum          Fix the file's checksum in the PE header if the file\n\
                           has been successfully rebased.  This also bumps the\n\
                           file's modification time (like -t) if the checksum\n\
-                          has been changed.  By default, the checksum is\n\
-                          updated without reading the whole file under the\n\
-                          assumption that the original checksum was correct.\n\
+                          has been changed.  By default, a set (nonzero)\n\
+                          checksum is updated without reading the whole file\n\
+                          under the assumption that the original checksum was\n\
+                          correct.\n\
   -d, --down              Treat the BaseAddress as upper ceiling and rebase\n\
                           files top-down from there.  Without this option the\n\
                           files are rebased from BaseAddress bottom-up.\n\
-- 
2.39.0