Bug 5075 - Syncing with --iconv may yield protocol error
Summary: Syncing with --iconv may yield protocol error
Status: CLOSED FIXED
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.0.0
Hardware: All All
: P3 major (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-12 00:06 UTC by Lennart Lövstrand
Modified: 2008-07-26 10:19 UTC (History)
0 users

See Also:


Attachments
Diffs to fix bug (2.56 KB, patch)
2007-11-12 00:14 UTC, Lennart Lövstrand
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lennart Lövstrand 2007-11-12 00:06:00 UTC
If you use --iconv to sync between two machines and a file with a name that needs conversion is to be deleted, the rsync protocol may get out of sync and yield a protocol error if the converted filename differs in length form the unconverted name.  This is because mplex_write in io.c will record and send the message length *before* the message has been converted using iconvbuf.  If the converted message differs in length, the reading rsync process will get confused and interpret part of the message data as tag & length.

Here is an example:

[21:45:27]lenux:~/Mirror$ rsync -a --progress --del -n --iconv=utf-8,utf-8-mac Lennart/Auto/ sva:Mirror/Lennart/Auto/
sending incremental file list
Audi Cabrio 92 (CUB931)/D\#303\#244ck/
deleting Audi Cabrio 92 (CUB931)/D\#303\#244ck/www.stro.nu.url.URL\#001
unexpected tag -7 [sender]
rsync error: error in rsync protocol data stream (code 12) at io.c(1135) [sender=3.0.0pre5]

(Note the garbage #\001 at the end of the filename.)

To fix this, mplex_write needs to convert the message data before sending its length.  Here is a possible rewrite:

/* Write an message to a multiplexed stream. If this fails, rsync exits. */
static void mplex_write(int fd, enum msgcode code, const char *buf, size_t len, int convert)
{
	char buffer[BIGPATHBUFLEN]; /* Oversized for use by iconv code. */
	size_t n = len;

#ifdef ICONV_OPTION
	if (convert && ic_send == (iconv_t)-1)
#endif
		convert = 0;

#ifdef ICONV_OPTION
	/* We need to convert buf before doing anything else so that we
	 * can include the (converted) byte length in the message header.
	 */
	if (convert) {
		xbuf outbuf, inbuf;

		INIT_XBUF(outbuf, buffer + 4, 0, sizeof(buffer) - 4);
		INIT_XBUF(inbuf, (char*)buf, len, -1);

		iconvbufs(ic_send, &inbuf, &outbuf,
				  ICB_INCLUDE_BAD | ICB_INCLUDE_INCOMPLETE);

		if (inbuf.len > 0) {
			rprintf(FERROR, "conversion buffer overflow in mplex_write; "
					"either enlarge 'buffer' or rewrite code to allocate "
					"buffer dynamically");
		}

		n = len = outbuf.len;
	} else
#endif
	if (n > sizeof(buffer) - 4)
		n = 0;
	else
		memcpy(buffer + 4, buf, n);

	SIVAL(buffer, 0, ((MPLEX_BASE + (int)code)<<24) + len);


	defer_forwarding_keep = 1; /* defer_forwarding_messages++ on return */
	writefd_unbuffered(fd, buffer, n+4);
	defer_forwarding_keep = 0;

	len -= n;
	buf += n;

	if (len)
		writefd_unbuffered(fd, buf, len);

	if (!--defer_forwarding_messages && !no_flush)
		msg_flush();
}

Context diff based on 3.0.0pre5 follows:

*** io.c~	Sat Nov  3 00:20:05 2007
--- io.c	Sun Nov 11 21:59:57 2007
***************
*** 468,485 ****
  	char buffer[BIGPATHBUFLEN]; /* Oversized for use by iconv code. */
  	size_t n = len;
  
- 	SIVAL(buffer, 0, ((MPLEX_BASE + (int)code)<<24) + len);
- 
  #ifdef ICONV_OPTION
  	if (convert && ic_send == (iconv_t)-1)
  #endif
  		convert = 0;
  
! 	if (convert || n > 1024 - 4) /* BIGPATHBUFLEN can handle 1024 bytes */
  		n = 0;
  	else
  		memcpy(buffer + 4, buf, n);
  
  	defer_forwarding_keep = 1; /* defer_forwarding_messages++ on return */
  	writefd_unbuffered(fd, buffer, n+4);
  	defer_forwarding_keep = 0;
--- 468,508 ----
  	char buffer[BIGPATHBUFLEN]; /* Oversized for use by iconv code. */
  	size_t n = len;
  
  #ifdef ICONV_OPTION
  	if (convert && ic_send == (iconv_t)-1)
  #endif
  		convert = 0;
  
! #ifdef ICONV_OPTION
! 	/* We need to convert buf before doing anything else so that we
! 	 * can include the (converted) byte length in the message header.
! 	 */
! 	if (convert) {
! 		xbuf outbuf, inbuf;
! 
! 		INIT_XBUF(outbuf, buffer + 4, 0, sizeof(buffer) - 4);
! 		INIT_XBUF(inbuf, (char*)buf, len, -1);
! 
! 		iconvbufs(ic_send, &inbuf, &outbuf,
! 				  ICB_INCLUDE_BAD | ICB_INCLUDE_INCOMPLETE);
! 
! 		if (inbuf.len > 0) {
! 			rprintf(FERROR, "conversion buffer overflow in mplex_write; "
! 					"either enlarge 'buffer' or rewrite code to allocate "
! 					"buffer dynamically");
! 		}
! 
! 		n = len = outbuf.len;
! 	} else
! #endif
! 	if (n > sizeof(buffer) - 4)
  		n = 0;
  	else
  		memcpy(buffer + 4, buf, n);
  
+ 	SIVAL(buffer, 0, ((MPLEX_BASE + (int)code)<<24) + len);
+ 
+ 
  	defer_forwarding_keep = 1; /* defer_forwarding_messages++ on return */
  	writefd_unbuffered(fd, buffer, n+4);
  	defer_forwarding_keep = 0;
***************
*** 487,506 ****
  	len -= n;
  	buf += n;
  
- #ifdef ICONV_OPTION
- 	if (convert) {
- 		xbuf outbuf, inbuf;
- 
- 		INIT_CONST_XBUF(outbuf, buffer);
- 		INIT_XBUF(inbuf, (char*)buf, len, -1);
- 
- 		do {
- 			iconvbufs(ic_send, &inbuf, &outbuf,
- 				  ICB_INCLUDE_BAD | ICB_INCLUDE_INCOMPLETE);
- 			writefd_unbuffered(fd, outbuf.buf, outbuf.len);
- 		} while (inbuf.len);
- 	} else
- #endif
  	if (len)
  		writefd_unbuffered(fd, buf, len);
Comment 1 Lennart Lövstrand 2007-11-12 00:14:19 UTC
Created attachment 2963 [details]
Diffs to fix bug
Comment 2 Lennart Lövstrand 2007-11-12 00:17:37 UTC
Sorry, I meant to add this too:

There is also an uninitialized variable in readfd_buffered that can cause garbage to be read independently of the bug in mplex_write.  Under case MSG_DELETED, inbuf.pos is never initialized which can -- and will -- cause the call to iconvbufs further down to process random data.

The fix is to initialize inbuf with a proper call to INIT_XBUF before proceeding to do the conversion.

Diff below and attached.

--- 510,515 ----
***************
*** 1060,1066 ****
  				int pos = 0;
  
  				INIT_CONST_XBUF(outbuf, line);
! 				inbuf.buf = ibuf;
  
  				while (msg_bytes) {
  					inbuf.len = msg_bytes > sizeof ibuf
--- 1069,1075 ----
  				int pos = 0;
  
  				INIT_CONST_XBUF(outbuf, line);
! 				INIT_XBUF(inbuf, ibuf, 0, -1);
  
  				while (msg_bytes) {
  					inbuf.len = msg_bytes > sizeof ibuf
Comment 3 Wayne Davison 2007-11-22 10:09:52 UTC
Thank you very much for the problem report and the patch.  Both problems are now fixed in the git version.