add ubsan to and cleanup undefined behaviour issues

.github/workflows/asan-build.yml

@@ -0,0 +1,72 @@
+name: rsync ASan+UBSan (clang)
+
+on:
+  push:
+    branches: [ master ]
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - '!.github/workflows/asan-build.yml'
+  pull_request:
+    branches: [ master ]
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - '!.github/workflows/asan-build.yml'
+  schedule:
+    # Weekly (Mon 09:42 UTC): catch breakage from a moving ubuntu-latest/clang
+    # toolchain (a new clang can add a UBSan check, or change ASan behaviour)
+    # that no code push would otherwise trigger.  Push/PR already gate every
+    # code change, so daily would just re-run an unchanged tree.
+    - cron: '42 9 * * 1'
+  workflow_dispatch:
+
+jobs:
+  asan:
+    runs-on: ubuntu-latest
+    name: rsync ASan+UBSan (clang)
+    env:
+      # rsync intentionally leaks small allocations at process exit, so leak
+      # detection would be all noise; chase only memory-safety errors.
+      ASAN_OPTIONS: detect_leaks=0:abort_on_error=1
+      # UBSan is a gate: -fno-sanitize-recover=undefined (below) aborts on the
+      # first finding and halt_on_error=1 makes that fatal, so any undefined
+      # behaviour fails the run.  This needs the tree to be UBSan-clean: the
+      # remaining findings are fixed in code (hashtable/mdfour shifts, xattrs,
+      # and log.c's file_struct, kept aligned via rounding.h); only byteorder.h's
+      # intentional unaligned accessors are suppressed, with no_sanitize.
+      UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+    - name: prep
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y clang acl libacl1-dev attr libattr1-dev liblz4-dev libzstd-dev libxxhash-dev openssl
+        echo "/usr/local/bin" >>"$GITHUB_PATH"
+    - name: configure
+      # -DNDEBUG builds as a shipped release does (assert() compiled out), so
+      # AddressSanitizer catches the over-reads/over-writes that an "assert()
+      # instead of a real bounds check" bug would cause in a production build.
+      # UBSan rides along on the same build; -fno-sanitize-recover=undefined
+      # makes any undefined behaviour abort (and thus fail the run) instead of
+      # merely printing it.
+      run: |
+        CC=clang \
+        CFLAGS="-fsanitize=address,undefined -fno-sanitize-recover=undefined -fno-omit-frame-pointer -g -O1 -DNDEBUG" \
+        LDFLAGS="-fsanitize=address,undefined" \
+        ./configure --with-rrsync --disable-md2man
+    - name: make
+      # check-progs builds rsync plus the test helper programs (tls, trimslash,
+      # t_unsafe, ...) that runtests.py requires; plain "make" builds only rsync
+      # and runtests aborts on the missing helpers.
+      run: make check-progs
+    - name: info
+      run: ./rsync --version
+    - name: check (stdio-pipe transport)
+      # ASan+UBSan-instrumented coverage of the transfer, daemon, sender,
+      # receiver and metadata paths over the default stdio-pipe transport.
+      run: ./runtests.py --rsync-bin="$PWD/rsync" -j8
+    - name: check (TCP daemon transport)
+      # --use-tcp also exercises the loopback rsyncd listener and the client's
+      # TCP connection path.
+      run: ./runtests.py --rsync-bin="$PWD/rsync" --use-tcp -j8

.github/workflows/scan-build.yml

@@ -0,0 +1,51 @@
+name: rsync scan-build (clang analyzer)
+
+on:
+  push:
+    branches: [ master ]
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - '!.github/workflows/scan-build.yml'
+  pull_request:
+    branches: [ master ]
+    paths-ignore:
+      - '.github/workflows/*.yml'
+      - '!.github/workflows/scan-build.yml'
+  workflow_dispatch:
+
+jobs:
+  scan-build:
+    runs-on: ubuntu-latest
+    name: rsync scan-build (clang analyzer)
+    steps:
+    - uses: actions/checkout@v4
+      with:
+        fetch-depth: 0
+    - name: prep
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y clang clang-tools acl libacl1-dev attr libattr1-dev liblz4-dev libzstd-dev libxxhash-dev openssl
+    - name: configure (under scan-build)
+      # Run configure under scan-build so its analyzer compiler-wrapper is baked
+      # into the Makefile's $(CC); --disable-md2man avoids the doc toolchain.
+      run: scan-build ./configure --with-rrsync --disable-md2man
+    - name: scan-build (informational)
+      # Static analysis only -- INFORMATIONAL, not a gate.  rsync currently has
+      # a fair number of reports that are overwhelmingly known false positives
+      # (e.g. unix.Chroot "no chdir after chroot", core.NonNullParamChecker
+      # against functions that can't actually receive NULL).  We publish the
+      # HTML report as an artifact and print the bug count to the run summary,
+      # but do NOT pass --status-bugs, so this surfaces new analyzer findings
+      # without going red on arrival.  check-progs builds rsync + the test
+      # helpers without needing the man-page toolchain.
+      run: |
+        scan-build -o "$PWD/scan-report" make check-progs -j"$(nproc)" 2>&1 | tee scan-build.out
+        echo '## scan-build summary' >>"$GITHUB_STEP_SUMMARY"
+        grep -E 'scan-build: .* bugs? found|scan-build: No bugs found' scan-build.out >>"$GITHUB_STEP_SUMMARY" || true
+    - name: upload report
+      if: always()
+      uses: actions/upload-artifact@v4
+      with:
+        name: scan-build-report
+        path: scan-report
+        if-no-files-found: ignore

Makefile.in

@@ -139,6 +139,7 @@ usage.o: version.h latest-year.h help-rsync.h help-rsyncd.h git-version.h defaul
 loadparm.o: default-dont-compress.h daemon-parm.h
 
 flist.o: rounding.h
+log.o: rounding.h
 
 default-cvsignore.h default-dont-compress.h: rsync.1.md define-from-md.awk
 	$(AWK) -f $(srcdir)/define-from-md.awk -v hfile=$@ $(srcdir)/rsync.1.md

byteorder.h

@@ -68,10 +68,26 @@ SIVAL64(char *buf, int pos, int64 val)
 
 #else /* !CAREFUL_ALIGNMENT */
 
+/* We don't want false positives about alignment from UBSAN, see:
+   https://github.com/WayneD/rsync/issues/427#issuecomment-1375132291
+*/
+
+/* From https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html */
+#ifndef GCC_VERSION
+#define GCC_VERSION (__GNUC__ * 10000 \
+                     + __GNUC_MINOR__ * 100 \
+                     + __GNUC_PATCHLEVEL__)
+#endif
+
 /* This handles things for architectures like the 386 that can handle alignment errors.
  * WARNING: This section is dependent on the length of an int32 (and thus a uint32)
  * being correct (4 bytes)!  Set CAREFUL_ALIGNMENT if it is not. */
 
+#ifdef __clang__
+__attribute__((no_sanitize("undefined")))
+#elif GCC_VERSION >= 409
+__attribute__((no_sanitize_undefined))
+#endif
 static inline uint32
 IVALu(const uchar *buf, int pos)
 {
@@ -83,6 +99,11 @@ IVALu(const uchar *buf, int pos)
 	return *u.num;
 }
 
+#ifdef __clang__
+__attribute__((no_sanitize("undefined")))
+#elif GCC_VERSION >= 409
+__attribute__((no_sanitize_undefined))
+#endif
 static inline void
 SIVALu(uchar *buf, int pos, uint32 val)
 {
@@ -94,6 +115,11 @@ SIVALu(uchar *buf, int pos, uint32 val)
 	*u.num = val;
 }
 
+#ifdef __clang__
+__attribute__((no_sanitize("undefined")))
+#elif GCC_VERSION >= 409
+__attribute__((no_sanitize_undefined))
+#endif
 static inline int64
 IVAL64(const char *buf, int pos)
 {
@@ -105,6 +131,11 @@ IVAL64(const char *buf, int pos)
 	return *u.num;
 }
 
+#ifdef __clang__
+__attribute__((no_sanitize("undefined")))
+#elif GCC_VERSION >= 409
+__attribute__((no_sanitize_undefined))
+#endif
 static inline void
 SIVAL64(char *buf, int pos, int64 val)
 {

hashtable.c

@@ -351,7 +351,7 @@ void *hashtable_find(struct hashtable *tbl, int64 key, void *data_when_new)
 */
 
 #define NON_ZERO_32(x) ((x) ? (x) : (uint32_t)1)
-#define NON_ZERO_64(x, y) ((x) || (y) ? (y) | (int64)(x) << 32 | (y) : (int64)1)
+#define NON_ZERO_64(x, y) ((x) || (y) ? (y) | (uint64_t)(x) << 32 | (y) : (int64)1)
 
 uint32_t hashlittle(const void *key, size_t length)
 {

lib/mdfour.c

@@ -89,8 +89,8 @@ static void copy64(uint32 *M, const uchar *in)
 	int i;
 
 	for (i = 0; i < MD4_DIGEST_LEN; i++) {
-		M[i] = (in[i*4+3] << 24) | (in[i*4+2] << 16)
-		     | (in[i*4+1] << 8)  | (in[i*4+0] << 0);
+		M[i] = ((uint32)in[i*4+3] << 24) | ((uint32)in[i*4+2] << 16)
+		     | ((uint32)in[i*4+1] << 8)  | ((uint32)in[i*4+0] << 0);
 	}
 }
 

log.c

@@ -22,6 +22,7 @@
 #include "rsync.h"
 #include "itypes.h"
 #include "inums.h"
+#include "rounding.h"	/* EXTRA_ROUNDING, so log_delete() aligns its file_struct */
 
 extern int dry_run;
 extern int am_daemon;

xattrs.c

@@ -295,8 +295,12 @@ static int rsync_xal_get(const char *fname, item_list *xalp)
 	rxa = xalp->items;
 	if (count > 1)
 		qsort(rxa, count, sizeof (rsync_xa), rsync_xal_compare_names);
-	for (rxa += count-1; count; count--, rxa--)
-		rxa->num = count;
+	/* Guard count==0: rxa is then xalp->items (possibly NULL) and the
+	 * "rxa += count-1" init would form NULL-1 (undefined). */
+	if (count) {
+		for (rxa += count-1; count; count--, rxa--)
+			rxa->num = count;
+	}
 	return 0;
 }
 
@@ -381,17 +385,19 @@ static int64 xattr_lookup_hash(const item_list *xalp)
 {
 	const rsync_xa *rxas = xalp->items;
 	size_t i;
-	int64 key = hashlittle2(&xalp->count, sizeof xalp->count);
+	/* Accumulate unsigned: the summed hash values routinely overflow a
+	 * signed int64 (UB), and we only care about the resulting bit pattern. */
+	uint64_t key = (uint64_t)hashlittle2(&xalp->count, sizeof xalp->count);
 
 	for (i = 0; i < xalp->count; i++) {
-		key += hashlittle2(rxas[i].name, rxas[i].name_len);
+		key += (uint64_t)hashlittle2(rxas[i].name, rxas[i].name_len);
 		if (rxas[i].datum_len > MAX_FULL_DATUM)
-			key += hashlittle2(rxas[i].datum, xattr_sum_len);
+			key += (uint64_t)hashlittle2(rxas[i].datum, xattr_sum_len);
 		else
-			key += hashlittle2(rxas[i].datum, rxas[i].datum_len);
+			key += (uint64_t)hashlittle2(rxas[i].datum, rxas[i].datum_len);
 	}
 
-	return key;
+	return (int64)key;
 }
 
 static int find_matching_xattr(const item_list *xalp)
@@ -460,7 +466,9 @@ static int rsync_xal_store(item_list *xalp)
 	 * entire initial-count, not just enough space for one new item. */
 	*new_list = empty_xa_list;
 	(void)EXPAND_ITEM_LIST(&new_list->xa_items, rsync_xa, xalp->count);
-	memcpy(new_list->xa_items.items, xalp->items, xalp->count * sizeof (rsync_xa));
+	/* xalp->items is NULL for an empty list; memcpy(dst, NULL, 0) is UB. */
+	if (xalp->count)
+		memcpy(new_list->xa_items.items, xalp->items, xalp->count * sizeof (rsync_xa));
 	new_list->xa_items.count = xalp->count;
 	xalp->count = 0;