diff --git a/.github/workflows/asan-build.yml b/.github/workflows/asan-build.yml new file mode 100644 index 000000000..9acc8d8f6 --- /dev/null +++ b/.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 diff --git a/.github/workflows/scan-build.yml b/.github/workflows/scan-build.yml new file mode 100644 index 000000000..23c1b73c8 --- /dev/null +++ b/.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 diff --git a/Makefile.in b/Makefile.in index 8e8b2ef4c..5216fb2f7 100644 --- a/Makefile.in +++ b/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 diff --git a/byteorder.h b/byteorder.h index 059cc7086..855e8fe14 100644 --- a/byteorder.h +++ b/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) { diff --git a/hashtable.c b/hashtable.c index 2cc4e5504..f4aa85f1c 100644 --- a/hashtable.c +++ b/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) { diff --git a/lib/mdfour.c b/lib/mdfour.c index 6203658d5..7df180618 100644 --- a/lib/mdfour.c +++ b/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); } } diff --git a/log.c b/log.c index b948f16a1..5f1f3a29c 100644 --- a/log.c +++ b/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; diff --git a/xattrs.c b/xattrs.c index 99795f244..0b971d543 100644 --- a/xattrs.c +++ b/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;