]> git.ipfire.org Git - thirdparty/rsync.git/commitdiff
receiver: fix NULL deref on the delta discard path
authorpterror <pterrorbird@gmail.com>
Fri, 5 Jun 2026 07:24:05 +0000 (17:24 +1000)
committerAndrew Tridgell <andrew@tridgell.net>
Sat, 6 Jun 2026 08:56:51 +0000 (18:56 +1000)
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 31fbb17d ("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 31fbb17d 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>
receiver.c

index 84bb151aa1b951fd41854488059ca878ccac2a18..2b263cea06adb878c2f5528c20576d8d37c76734 100644 (file)
@@ -423,16 +423,32 @@ static int receive_data(int f_in, char *fname_r, int fd_r, OFF_T size_r,
 
                stats.matched_data += len;
 
-               /* A block match can only be honored if we actually mapped the
-                * basis. If we didn't (basis open failed), the sender should
-                * never have been told a basis existed -- treat it as a protocol
-                * inconsistency rather than silently omitting these bytes from
-                * the verification checksum (which yields a spurious failure) or
-                * leaving a hole in the output. */
+               /* A block match with no mapped basis is a protocol inconsistency
+                * ONLY when we are actually producing output (fd != -1): the
+                * generator told the sender a basis existed but the receiver could
+                * not open it, so honoring the match would silently omit these
+                * bytes from the verification checksum (a spurious failure) or
+                * leave a hole in the output. Fail cleanly in that case.
+                *
+                * On the DISCARD path (fd == -1, fname == NULL) there is no output
+                * and no verification: discard_receive_data() deliberately drains a
+                * delta the receiver never intends to write (basis fstat failed,
+                * basis is a directory, output open failed, batch skip, ...). The
+                * sender does not know the data is being discarded and streams an
+                * ordinary delta, so a match token here is NORMAL protocol, not
+                * malformed. Absorb it benignly (advance the offset and continue),
+                * as the pre-existing "if (mapbuf)" guards did before this check was
+                * added in 31fbb17d -- erroring would wrongly break legitimate
+                * transfers, and full_fname(fname) with fname==NULL would
+                * dereference NULL (a receiver crash on a normal transfer). */
                if (!mapbuf) {
-                       rprintf(FERROR, "got a block match with no basis file for %s [%s]\n",
-                               full_fname(fname), who_am_i());
-                       exit_cleanup(RERR_PROTOCOL);
+                       if (fd != -1) {
+                               rprintf(FERROR, "got a block match with no basis file for %s [%s]\n",
+                                       full_fname(fname), who_am_i());
+                               exit_cleanup(RERR_PROTOCOL);
+                       }
+                       offset += len;
+                       continue;
                }
 
                if (DEBUG_GTE(DELTASUM, 3)) {