vcs-svn: make reading of properties binary-safe

svn-fe errors out on revision 59151 of the ASF repository:

 fatal: invalid dump: unexpected end of file

The proximate cause is a property with an embedded NUL character.
Previously such anomalies were ignored but commit c9d1c8ba
(2010-12-28) introduced a check strlen(val) == len to avoid reading
uninitialized data when a property list ends early and unfortunately
this test does not distinguish between "foo" followed by EOF and the
string "foo\0bar\0baz".

Fix it by using buffer_read_binary to read to a strbuf and checking
the actual length read.  Most consumers of properties still use
C-style strings, so in practice an author or log message with embedded
NULs will be truncated, but a least this way svn-fe won't error out
(fixing the regression).

Reported-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index 5a6a4b9b7a6e6abbb9570ddceaac68baf5c2690b..5e2b123080747a77f8004c6cfaef9fcb7eb9e26f 100755 (executable)
--- 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 'NUL in property value' '
+       reinit_git &&
+       echo "commit message" >expect.message &&
+       {
+               properties \
+                       unimportant "something with a NUL (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
+       } >nulprop.dump &&
+       test-svn-fe nulprop.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 ea5b128e4f205cd95694839ca9eb9b072333fa99..c00f031179702ff4dd96696e79a9274322967c44 100644 (file)
--- 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: