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

From: tb <[email-masked]>
Date: Mon, 28 Mar 2011 17:34:02 +0200

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<[email-masked]>
 > Signed-off-by: Jonathan Nieder<[email-masked]>
 > ---
 >   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:
> 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<[email-masked]>
> Signed-off-by: Jonathan Nieder<[email-masked]>
> ---
>   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: