Bug 5201 - Rsync lets user corrupt dest by applying non-inplace batch in inplace mode
Summary: Rsync lets user corrupt dest by applying non-inplace batch in inplace mode
Status: CLOSED FIXED
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.0.0
Hardware: Other Linux
: P3 normal (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-14 16:01 UTC by Matt McCutchen
Modified: 2008-07-26 10:28 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matt McCutchen 2008-01-14 16:01:18 UTC
I originally reported this bug here:

http://lists.samba.org/archive/rsync/2007-December/019369.html

I noticed that rsync will let me apply a non-inplace batch file in inplace mode.  This corrupts the destination file if the batch file copies any data forward (from earlier offsets to later ones).  Of course, the post-transfer checksum detects the corruption and gives the "ERROR: <file> failed verification" message, but rsync doesn't give the user a clue why the corruption occurred.

I think rsync should at least warn the user and possibly refuse outright to apply a non-inplace batch in inplace mode.  However, existing batch
files don't indicate which mode they were written in, and it would be nice to let a user apply a non-inplace batch if it happened not to use
any forward copies.  Thus, the best thing to do might be to issue an error like this the first time a forward copy is seen for each file
(followed, of course, by the post-transfer checksum failure):

rsync: error: batch file delta for <file> contains a forward copy, which cannot be performed in --inplace mode (probably the batch file was written without --inplace); the update will probably be corrupted

Here is a script that demonstrates the current behavior:

#!/bin/bash
rm -rf src dest batch*
mkdir src dest
wget http://rsync.samba.org/ftp/rsync/rsync-3.0.0pre8.tar.gz -O dest/file
head -c 1000000 /dev/zero | cat - dest/file >src/file
rsync --no-whole-file --only-write-batch=batch src/file dest/
rsync --inplace --read-batch=batch dest/

Output:

ERROR: file failed verification -- update retained.
(No batched update for "file")

Two additional problems seem to have arisen since my original mail to the list:

1. The inaccurate message `(No batched update for "file")' appears.  Furthermore, in the case of a solo file, this message shows its source name while the previous line shows its destination name, which seems inconsistent.

2. Rsync isn't exiting with code 23, which might mislead a script to think that the copy was successful.
Comment 1 Wayne Davison 2008-01-19 13:27:01 UTC
I added option checking for --inpace and several others that need to be set correctly for the proper reading of the batch file.
Comment 2 Matt McCutchen 2008-01-23 23:27:41 UTC
With --read-batch, when rsync processes the flags, the protocol_version is always 30 (it has not yet been set from the batch file).  As a result, --inplace is incorrectly forced off for any pre-protocol-30 batch file, regardless of whether the batch file was written with --inplace.

Once the above is fixed, I am still hoping that rsync 3 will fail more gracefully when applying a *pre-protocol-30* non-inplace batch file in inplace mode.
Comment 3 Wayne Davison 2008-01-26 16:28:39 UTC
I fixed the problem with the parsing of the batch flags before the protocol_version of the batch file is known.

As for further checking, I don't think I'll add anything else.  The batch.sh file has the options they should use to parse the batch file.  If they mess things up, it's good for us to try to help them out, but I don't want to clutter the transfer code with sanity checks for startup options.
Comment 4 Matt McCutchen 2008-01-26 17:09:27 UTC
(In reply to comment #3)
> I don't want to
> clutter the transfer code with sanity checks for startup options.

That's reasonable, but the lack of an exit code 23 when the post-transfer checksum fails still needs to be fixed.  An rsync 2.6.9 receiver gets this right, and it also prints `(No batched update for resend of "file")' (note the `resend of').  Perhaps the same code path does both the `resend of' and the exit code 23, and it just isn't being triggered properly in the rsync 3.0.0 receiver.
Comment 5 Wayne Davison 2008-01-27 16:44:17 UTC
OK, I missed the extra errors at the end of your report the first time around.  They should now be fixed (as well as a couple other minor glitches in the same area).  Thanks!
Comment 6 Matt McCutchen 2008-01-27 22:11:29 UTC
Wow!  This is actually completely fixed!
Comment 7 Matt McCutchen 2008-01-27 22:14:43 UTC
If it is safe to apply an inplace batch file in non-inplace mode, it would be nice to allow this.