Re: [PATCH 1/4] vcs-svn: make reading of properties binary-safe

Hej, I'm not sure if this was the origin email ...

Commit e7d04ee147dcbe6af1fa1d2147466696e is OK.

But: failure on t9010 with commit 195b7ca6f229455da61f9f6b ============= # test_cmp expect.message actual.message && # test_cmp expect.hello1 actual.hello1 && # test_cmp expect.hello2 actual.hello2 # ok 14 - change file mode and reiterate content ok 15 - deltas not supported ok 16 - property deltas supported ok 17 - properties on / ok 18 - deltas for typechange ok 19 - set up svn repo ok 20 - t9135/svn.dump # still have 3 known breakage(s) # failed 1 among remaining 17 test(s) 1..20 =====================

Some more info: b@birne:~/projects/git/git.git> uname -a Darwin birne.lan 10.7.0 Darwin Kernel Version 10.7.0: Sat Jan 29 15:17:16 PST 2011; root:xnu-1504.9.37~1/RELEASE_I386 i386

tb@birne:~/projects/git/git.git> svn --version
svn, version 1.6.15 (r1038135)
    compiled Jan 29 2011, 15:18:15

tb@birne:~/projects/git/git.git> svnadmin --version
svnadmin, version 1.6.15 (r1038135)
    compiled Jan 29 2011, 15:18:15

  which svn
/usr/bin/svn

I can assist with some more testing BR /Torsten

On 03/25/2011 05:07 AM, Jonathan Nieder wrote:
 > A caller to buffer_read_string cannot easily tell the difference
 > between the string "foo" followed by an early end of file and the
 > string "foo\0bar\0baz".  In a half-hearted attempt to catch early EOF,
 > c9d1c8ba (2010-12-28) introduced a safety strlen(val) == len for
 > property keys and values, to at least keep svn-fe from reading
 > uninitialized data when a property list ends early due to EOF.
 >
 > But it is permissible for both keys and values to contain null
 > characters, so in handling revision 59151 of the ASF repository svn-fe
 > encounters a null byte and produces the following message:
 >
 >   fatal: invalid dump: unexpected end of file
 >
 > Fix it by using buffer_read_binary to read to a strbuf (and keep track
 > of the actual length read).  Most consumers of properties still use
 > C-style strings, so in practice we still can't use an author or log
 > message with embedded nuls, but at least this way svn-fe won't error
 > out.
 >
 > Reported-by: David Barr<david.barr@cordelta.com>
 > Signed-off-by: Jonathan Nieder<jrnieder@gmail.com>
 > ---
 >   t/t9010-svn-fe.sh |   27 +++++++++++++++++++++++++++
 >   vcs-svn/svndump.c |   24 ++++++++++--------------
 >   2 files changed, 37 insertions(+), 14 deletions(-)
 >
 > diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
 > index 5a6a4b9..47f1e4f 100755
 > --- a/t/t9010-svn-fe.sh
 > +++ b/t/t9010-svn-fe.sh
 > @@ -370,6 +370,33 @@ test_expect_failure 'change file mode but keep 
old content' '
 >   	test_cmp hello actual.target
 >   '
 >
 > +test_expect_success 'null byte in property value' '
 > +	reinit_git&&
 > +	echo "commit message">expect.message&&
 > +	{
 > +		properties \
 > +			unimportant "something with a null byte (Q)" \
 > +			svn:log "commit message"&&
 > +		echo PROPS-END
 > +	} |
 > +	q_to_nul>props&&
 > +	{
 > +		cat<<-\EOF&&
 > +		SVN-fs-dump-format-version: 3
 > +
 > +		Revision-number: 1
 > +		EOF
 > +		echo Prop-content-length: $(wc -c<props)&&
 > +		echo Content-length: $(wc -c<props)&&
 > +		echo&&
 > +		cat props
 > +	}>nullprop.dump&&
 > +	test-svn-fe nullprop.dump>stream&&
 > +	git fast-import<stream&&
 > +	git diff-tree --always -s --format=%s HEAD>actual.message&&
 > +	test_cmp expect.message actual.message
 > +'
 > +
 >   test_expect_success 'change file mode and reiterate content' '
 >   	reinit_git&&
 >   	cat>expect<<-\EOF&&
 > diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
 > index ea5b128..c00f031 100644
 > --- a/vcs-svn/svndump.c
 > +++ b/vcs-svn/svndump.c
 > @@ -147,6 +147,7 @@ static void die_short_read(void)
 >   static void read_props(void)
 >   {
 >   	static struct strbuf key = STRBUF_INIT;
 > +	static struct strbuf val = STRBUF_INIT;
 >   	const char *t;
 >   	/*
 >   	 * NEEDSWORK: to support simple mode changes like
 > @@ -163,15 +164,15 @@ static void read_props(void)
 >   	uint32_t type_set = 0;
 >   	while ((t = buffer_read_line(&input))&&  strcmp(t, "PROPS-END")) {
 >   		uint32_t len;
 > -		const char *val;
 >   		const char type = t[0];
 >   		int ch;
 >
 >   		if (!type || t[1] != ' ')
 >   			die("invalid property line: %s\n", t);
 >   		len = atoi(&t[2]);
 > -		val = buffer_read_string(&input, len);
 > -		if (!val || strlen(val) != len)
 > +		strbuf_reset(&val);
 > +		buffer_read_binary(&input,&val, len);
 > +		if (val.len<  len)
 >   			die_short_read();
 >
 >   		/* Discard trailing newline. */
 > @@ -179,22 +180,17 @@ static void read_props(void)
 >   		if (ch == EOF)
 >   			die_short_read();
 >   		if (ch != '\n')
 > -			die("invalid dump: expected newline after %s", val);
 > +			die("invalid dump: expected newline after %s", val.buf);
 >
 >   		switch (type) {
 >   		case 'K':
 > +			strbuf_swap(&key,&val);
 > +			continue;
 >   		case 'D':
 > -			strbuf_reset(&key);
 > -			if (val)
 > -				strbuf_add(&key, val, len);
 > -			if (type == 'K')
 > -				continue;
 > -			assert(type == 'D');
 > -			val = NULL;
 > -			len = 0;
 > -			/* fall through */
 > +			handle_property(&val, NULL, 0,&type_set);
 > +			continue;
 >   		case 'V':
 > -			handle_property(&key, val, len,&type_set);
 > +			handle_property(&key, val.buf, len,&type_set);
 >   			strbuf_reset(&key);
 >   			continue;
 >   		default:

======================

On 03/25/2011 05:07 AM, Jonathan Nieder wrote: