receiver: fix NULL deref on the delta discard path#966
Merged
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
receive_data() crashed a receiver that was merely DISCARDING a file's delta stream. discard_receive_data() calls receive_data() with fname == NULL and fd == -1, so size_r == 0 and mapbuf == NULL. A normal block-MATCH token (against a block the basis and source share) then reaches the !mapbuf branch added in 31fbb17 ("receiver: fix absolute --partial-dir delta resume"), which calls full_fname(fname). full_fname() dereferences its argument unconditionally (util1.c: `if (*fn == '/')`), so fname == NULL faults there -> receiver SIGSEGV. This is a normal-operation crash with a stock cooperating sender, not an adversarial one. The generator hands the sender real block sums whenever the basis is readable and we're in delta mode; the receiver only decides to discard afterwards, when its output cannot be produced -- e.g. the destination directory is not writable (mkstemp fails), the basis turns out to be a directory, or a --partial-dir resume is skipped. A MATCH token arriving during that discard hit the NULL deref. The 31fbb17 branch is correct only for a REAL output transfer (fd != -1, fname valid): there, a block match with no mapped basis is a genuine protocol inconsistency (the generator promised a basis the receiver could not open), and honoring it would silently omit those bytes from the verification checksum or leave a hole, so hard-erroring -- and full_fname(fname) -- is right. It conflated that with the discard path. The discriminator is fd, not mapbuf: on the discard path fd == -1 always; on the real-output inconsistency fd != -1. Scope the "no basis file" protocol error to fd != -1 (where fname is non-NULL and full_fname is safe) and, on the discard path (fd == -1), absorb the matched bytes benignly (offset += len; continue) -- symmetric with the literal-token handling just above, and restoring the pre-31fbb17d behavior. The real-transfer inconsistency check is preserved unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drives a real sender<->receiver pair (client sender -> daemon receiver, both the binary under test in the default pipe transport) so the receiver actually takes the recv_files discard path -- a local `rsync a b` does not. The basis and source share a leading block so the generator emits real sums and the receiver gets a block MATCH; the destination directory is made unwritable so the receiver's output mkstemp() fails and it discards the delta. Pre-fix the receiver SIGSEGVs in full_fname(NULL), which the client sees as a protocol-data-stream error (code 12); post-fix it drains the delta and reports a benign code 23 (or 0). Skips (exit 77) when run as root, since root bypasses DAC and the unwritable destination would not make mkstemp() fail -- so the discard path, and the bug, would never be reached. Verified red-on-buggy / green-on-fixed against the 0d0399b receiver. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The regression test honestly skips when it cannot force the receiver's output mkstemp() to fail -- as root (root bypasses DAC) and on Cygwin (chmod 0555 does not deny the owner a write). The ubuntu, ubuntu-22.04, almalinux and macOS jobs run `make check` as root, and Cygwin can't enforce the unwritable directory, so the test skips on all of them. runtests.py fails a run on any skip-set mismatch, so add the test to those jobs' RSYNC_EXPECT_SKIPPED lists; the BSD/Solaris jobs run as root too but enforce no expected-skip set, so they need no change. Also tighten the pass condition. The post-chmod writability probe already guarantees the receiver discards (mkstemp must fail), so an exit 0 would mean the file actually transferred and the discard path was never exercised -- a silent false-pass. Require exactly exit 23 (the forced discard leaves the file untransferred); 12 remains the pre-fix crash.
tridge
force-pushed
the
fix-recv-discard-nullderef
branch
from
ddee5d0 to
fef4dea
Compare
Member
|
@pterror I've pushed some fixes that hopefully with get CI passing |
steadytao
approved these changes
Jun 6, 2026
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
3 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.
receive_data()can dereference NULL and crash the receiver on the discard path. When the receiver discards an incoming file's data —discard_receive_data()callsreceive_data()withfname == NULL,fd == -1, somapbuf == NULL— and the sender sends a block-match token, theif (!mapbuf)branch added in31fbb17dcallsfull_fname(fname)withfname == NULL;full_fname()derefs unconditionally → SIGSEGV.No malice needed — it happens in ordinary operation: any delta transfer where the basis is readable (so the generator sends real checksums and a stock sender emits match tokens) but the receiver can't create its output temp file — a push to a read-only / over-quota / out-of-inodes destination dir, a destination that is a directory where a file is expected, or a
--partial-dirresume. The receiver child SIGSEGVs; the transfer dies withcode 12.Fix: discriminate on
fd, notmapbuf. The error is only meaningful for the real-transfer caller (validfd/fname) — emit it (with the now-safefull_fname(fname)) only whenfd != -1; on the discard path (fd == -1) absorb the matched bytes (offset += len; continue;), restoring pre-31fbb17dbehavior.Adds
testsuite/recv-discard-nullderef_test.py— reproduces it deterministically over a daemon sender/receiver pair (shared-block basis + unwritable destination dir to force the discard path), asserts the receiver doesn't crash; fails (SIGSEGV) on current master, passes with the fix.Found via LLM-assisted fuzzing; fix and test written with LLM assistance.