add ubsan to and cleanup undefined behaviour issues#988
Open
tridge wants to merge 7 commits into
Open
Conversation
UBSan flags two spots that shift a value into the top bits of a word via a
signed operand:
* lib/mdfour.c copy64(): `in[i] << 24` promotes the uchar to int, so a
byte >= 128 overflows int (UB). Cast each byte to uint32.
* hashtable.c NON_ZERO_64(): `(int64)(x) << 32` overflows int64 whenever
x's high bit is set. Shift as uint64_t (covers all four call sites).
Behavior-preserving -- only the intermediate type changes; the resulting
bit pattern is identical.
Three pre-existing issues UBSan flags during the xattr tests:
* xattr_lookup_hash(): the summed hashlittle2() values overflow the
signed int64 accumulator (UB). Accumulate in uint64_t and convert back
at return -- the key is only used for hash-table equality, so the value
is unchanged.
* rsync_xal_get(): for an empty list (count == 0) the loop init
`rxa += count-1` forms `items - 1` on a NULL `items` (UB). Guard with
`if (count)`.
* rsync_xal_store(): `memcpy(dst, xalp->items, 0)` passes a NULL source for
an empty list (UB). Guard with `if (xalp->count)`.
log_delete()/log_formatted() dereference a struct file_struct that is built inside a char buffer offset by the (EXTRA_LEN-granular) extra data, so the pointer can be less-aligned than the struct's members. That unaligned access is intentional and harmless on the architectures rsync supports (others set CAREFUL_ALIGNMENT), but UBSan's alignment check flags it. Add a NO_SANITIZE_ALIGN attribute (granular "alignment" on clang / gcc 8+, the broader no_sanitize_undefined on gcc 4.9-7) and apply it to the two functions that touch such a pointer.
rsync sets CAREFUL_ALIGNMENT for architectures which do not support unaligned access. Disable UBSAN for functions which may use unaligned accesses when CAREFUL_ALIGNMENT is set. Bug: RsyncProject#427 Signed-off-by: Sam James <sam@gentoo.org> (cherry picked from commit 11c1e93)
The cherry-picked RsyncProject#428 wrapped no_sanitize attributes on read_varint() and read_varlong() in `#ifndef CAREFUL_ALIGNMENT`, but byteorder.h always #defines CAREFUL_ALIGNMENT (to 0 or 1), so that guard is never true and the attributes were dead code. They are also unnecessary: both functions read the assembled value through an aligned union member (union { char b[5]; int32 x; }), not an unaligned cast, so UBSan's alignment check never fires there (verified: the ASan+UBSan suite is clean without them). Remove the whole block rather than fix the guard. (The byteorder.h annotations from RsyncProject#428, which are real and correctly placed inside the !CAREFUL_ALIGNMENT branch, are kept.)
Add a clang AddressSanitizer + UndefinedBehaviorSanitizer workflow that builds rsync with -fsanitize=address,undefined -fno-sanitize-recover=undefined -DNDEBUG and runs the full test suite over both the stdio-pipe and TCP daemon transports. UBSAN_OPTIONS=halt_on_error=1 together with -fno-sanitize-recover=undefined makes any undefined behaviour fatal, so this job gates: the tree must stay UBSan-clean. The intentional unaligned accessors in byteorder.h are suppressed with no_sanitize and log.c's file_struct-in-buffer accessors with NO_SANITIZE_ALIGN; the rest of the tree is clean. -DNDEBUG builds as a release does (assert() compiled out) so ASan covers the production code paths. Runs on push/PR to master, nightly, and via workflow_dispatch.
Run the clang static analyzer over a check-progs build, publish the HTML report as an artifact, and print the bug count to the run summary. INFORMATIONAL only: it does not pass --status-bugs, so it surfaces new analyzer findings without going red on the existing (overwhelmingly false-positive) reports. Runs on push/PR to master, nightly, and via workflow_dispatch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This brings in an ancient PR #428 from @thesamesam and also fixes some UB issues that ubsan finds.
It also adds a gating CI check so we lock in the ubscan result, and a non-gating clang scan-build
Fixes #427 #428