[seks99x]

Summary

This PR resolves the sparse-file performance regression reported in #1115 (introduced by the MAX_CHAIN_LEN cap for #217) and bounds the CPU work spent in hash_search() when rolling across regions with repeated weak-checksum collisions.

Root Cause

1. Duplicate entries in a hash chain exhausting MAX_CHAIN_LEN (rsync 3.5.0 is very slow and uses a lot of CPU compared to 3.4.x with large sparse files #1115):
   When transferring large sparse or disk image files (where the block size reaches 64 KiB or 128 KiB), long runs of identical blocks (such as 0x00 sparse holes) land on the same hash chain as other constant-byte blocks (such as 0xFF) due to get_checksum1() arithmetic modulo 65536. Because build_hash_table() inserts blocks in sequential order, thousands of identical 0x00 blocks sit at the front of the chain. When hash_search() looks for a block sitting behind them, it walks and memcmps every identical 0x00 entry until hitting MAX_CHAIN_LEN (1024), aborting before it can reach valid matching blocks further down the chain.

2. Per-byte strong-checksum evaluation on unmatched collision runs:
   When a chain fails to produce a strong-checksum match (either after exhausting MAX_CHAIN_LEN or after computing get_checksum2()), hash_search() falls back to rolling forward 1 byte at a time. Across a constant-byte or colliding region that does not exist in the basis file, this forces a full get_checksum2() calculation at every single byte offset.

Changes

• Unlink duplicate false-alarm nodes (last_false_sum2):
  When a chain entry fails the strong-checksum comparison, record its sum2 pointer in last_false_sum2. Subsequent entries in the same chain that share the exact same sum1, len, and sum2 (and do not carry SUMFLG_SAME_OFFSET in --inplace mode) are unlinked from the chain (*prev = s->sums[i].chain) without incrementing chain_len. One representative of that block remains in the chain for future matches (with want_i / aligned_i continuing to resolve sequential indices directly from s->sums[]), while eliminating redundant memcmp calls and allowing hash_search() to reach valid blocks behind long duplicate runs.
• Skip block window after 4 consecutive false-alarm offsets (consecutive_false_offsets):
  If 4 consecutive 1-byte rolling offsets hit a weak-checksum collision (done_csum2 or chain_len > MAX_CHAIN_LEN) without matching any block in the chain, advance offset by a full block window (k - 1 prior to the loop's ++offset) rather than computing a strong checksum on every byte.
• Regression test (testsuite/collision_test.py):
  Verifies both duplicate-chain unlinking (rsync 3.5.0 is very slow and uses a lot of CPU compared to 3.4.x with large sparse files #1115) and bounded hash_hits on consecutive unmatched weak-checksum collisions.

Note on Even Block Sizes and Repeated Sequences

Because get_checksum1() accumulates s1 and s2 modulo $65536$, large power-of-2 / even block sizes (such as 65536 and 131072) can cause distinct constant-byte sequences to share the same 32-bit weak checksum. When rolling across unmatched repeated-byte sequences under an even block size, repeated weak collisions will trigger the consecutive-collision skip and transfer the colliding window as literal data. Users transferring files dominated by distinct constant-byte runs who want to avoid weak-checksum collisions altogether can specify an odd --block-size (e.g., --block-size=131071), which is coprime to $65536$ and gives all 256 constant-byte fills distinct weak checksums.

Many thanks to @samueloph and @dfskoll for reporting the MAX_CHAIN_LEN regression.

[steadytao]

Four misses do not prove that the next offsets cannot match. This skips valid basis matches and can substantially increase literal data. Split out the #1115 chain correction; any collision-flood policy needs explicit CPU and bandwidth bounds plus regressions with valid matches at each skip boundary. Also cover --inplace same-offset preservation when mutating the chain.

[seks99x]

This doesn't trigger on 4 normal misses. It only triggers on 4 consecutive 32-bit weak-checksum collisions (⁠done_csum2 == 1⁠) where the strong checksum failed to match every time. On normal/random data hitting 32 bytes weak sum collision 4 bytes at a row is practically impossible, so the data is either not matching already or this is a crafted checksum. Also, as noted in the PR description, on files with distinct repeated-byte runs, large even block sizes (like 64K/128K) wrap modulo 65536 to the same weak checksum (⁠0⁠), which can trigger this skip and send the colliding window as literal data, so using an odd ⁠--block-size⁠ is better there to avoid weak-checksum collisions on repeated bytes altogether. In both cases, there is no other way to stop the sender from pegging the CPU: if we don't drop the colliding block and jump ⁠offset⁠ to the next block, ⁠hash_search()⁠ is forced to walk 1 byte at a time and compute a full 64K–128K strong checksum (⁠get_checksum2()⁠) on every single byte O(NxB) , which still hangs the sender even when the chain length is 1.

I’ll broader the test coverage.

[steadytao]

Hmmmmm the skip can jump over valid matches and turn those regions into literal data. That might be an acceptable trade-off for separate collision-flood hardening but it probably should not be bundled with the regression fix. Please split out the duplicate-chain correction first.

[seks99x]

Another inplace (updating_basis_file) edge-case/max-chain regression remained. When aligned duplicate blocks (0x00 sparse runs) match in-place, they all receive SUMFLG_SAME_OFFSET and bypass chain deduplication. When more than MAX_CHAIN_LEN pinned duplicates accumulate, they exhaust the chain limit, spike false_alarms, and block valid matches further down the chain (causing extra literal data to be sent instead of pegging the cpu due to the collision limit). Fix this by tracking last_false_flags so that once a SUMFLG_SAME_OFFSET node is retained as last_false_sum2, redundant duplicate nodes carrying SUMFLG_SAME_OFFSET are unlinked as well.

[steadytao]

See previous review comments. But overall neither issue really warrants a CVE on the evidence here. A performance regression and the collision case only shows one per-connection sender process doing excessive work; it does not demonstrate memory corruption, cross-connection impact or daemon loss. Thank you for your work thus far nonetheless.

[seks99x]

@steadytao To put the literal-data concern in perspective: the max chain limit currently in master already skips vastly more valid basis matches on repeated-sequence files than consecutive_false_offsets >= 4 with dup-chain removal or current patch.

Because build_hash_table() inserts blocks in reverse order (LIFO), any sparse file with > 1024 zero blocks (just 128 MiB of zeros at 128 KiB block size) pushes earlier colliding blocks past index 1024. On Samuel's reported sparse benchmarks, MAX_CHAIN_LEN blindly amputates those valid basis blocks and more literal data was sent.

- small: 3.4.1 0.4 s, 0 literal; 3.5.1 13 s, 1 MiB literal
- reverse: 3.4.1 0.4 s, 0 literal; 3.5.1 2 min, 8 MiB literal
- large: 3.4.1 11 s, 0 literal; 3.5.1 69 s, 4 MiB literal

Current patch would result in 0 literal data as the old versions without all that cpu cost. It skips a match if a block is synthetically crafted with a bunched Adler-32 perturbation (+1, -2, +1 at bytes 0..2) inside the exact same fill byte as an adjacent collision flood.

Also, consecutive_false_offsets is not optional "hardening" here, it is the missing half of the exact same hash_search() issue (CVE-2026-70453). At every colliding byte offset across N bytes, hash_search() pays two costs:
Total CPU work = O(Nx(B+C))
where C is the chain walk (memcmp per node) and B is get_checksum2() hashing the full 64K–128K block window.

Here is how the 3 states compare:

1. 3.4.1 (Before rsync hangs with some specific files, using 100% of 1 CPU core but without disk nor network activity #217): Both C and N x B were unbounded. However, on aligned sparse files where valid blocks existed in the basis, 3.4.1 matched on block boundaries (0.4s on small and reverse, 0 literal). It only hung when an unmatched colliding region ( like a src 10 MB 0xFF and a dest 10MB 0x00 with block size 128KB ) forced it to walk the full chain (which have no limit) and calculate strong hash (which have no limit) at every single byte offset.
2. Current 3.5.0 / 3.5.1 (MAX_CHAIN_LEN only): Capped C at 1024 without unlinking duplicates, and left N x B completely unfixed. Because truncating the chain at 1024 breaks valid block matches on sparse files (rsync 3.5.0 is very slow and uses a lot of CPU compared to 3.4.x with large sparse files #1115), it forces hash_search() off the block boundaries and into the 1-byte sliding walk, calculating strong hash (which have no limit) and walking on the limited 1024 chain nodes on every single byte.
3. Why both fixes belong together in this PR:
   • Duplicate-chain unlinking fixes the C axis by collapsing duplicate basis runs to 1 representative node so valid blocks behind them are never cut off by MAX_CHAIN_LEN.
   • Consecutive collision skip (consecutive_false_offsets >= 4) fixes the NxB axis when rolling across unmatched colliding bytes.
     If we merge duplicate-chain unlinking alone without the offset skip, the collision CPU hang in hash_search() is still not fixed. Even when duplicate chains are collapsed to C = 1 node, rolling across an unmatched colliding run (such as crafted colliding blocks that are not matched) still triggers done_csum2 at every single byte offset leads to computing a 128 KiB strong checksum 1,048,576 times per megabyte O(NxB).

I actually thought you meant split the tests, but i combined them in one PR so we actually finish fixing the algorithmic complexity issues in hash_search() (#217 / CVE-2026-70453) and the #1115 regression (which is one of the incomplete-fix of the old CVE) together in one pass, rather than leaving the collision loop wide open , so what do you think about splitting?
I think there is mis-classification of the issue because this is not a memory corruption , but it does have a real impact and can lead to daemon loss not just performance issue. I'll send you reproducers for each case on discord.

[steadytao]

Hmm. Fair point, good to merge.

[samueloph]

Thanks for the fix.

@steadytao

But overall neither issue really warrants a CVE on the evidence here

On the CVE question: I agree the #1115 slowdown on legitimate data doesn't need one by itself. The per-offset strong-checksum cost is a different matter, though. SECURITY.md describes CVE-2026-70453 (rated HIGH) as preventing a crafted checksum set from making the sender "pin one connection's CPU" in hash_search(). Its fix bounded the chain walk, but the same weak-checksum collisions could still force a full get_checksum2() at every byte offset, which this PR now mitigates. That makes this an incomplete fix of CVE-2026-70453, and I think it should get its own CVE for consistency with the original classification.

It would also help downstream. The 70453 cap was backported to the v3.4.1-sec-patches3/4 and v3.2.7-sec-patches3/4 branches, so distributions that shipped it need an identifier to track this follow-up fix.

[steadytao]

Hmmm. Will speak to tridge on this one.

[steadytao]

@tridge - for tracking

[seks99x]

I agree, this still pin one connections CPU so it needs an independent CVE for tracking the complete fix of hash_search().