From e7d04ee147dcbe6af1fa1d2147466696e2be31bc Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 26 Mar 2011 00:15:10 -0500 Subject: 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 Signed-off-by: Jonathan Nieder --- vcs-svn/svndump.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'vcs-svn') diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c index ea5b128e4..c00f03117 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: -- cgit v1.2.1 From 7e2fe3a9fc816391b322ad9b3f2adf9342631db6 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 24 Mar 2011 23:09:19 -0500 Subject: vcs-svn: remove buffer_read_string All previous users of buffer_read_string have already been converted to use the more intuitive buffer_read_binary, so remove the old API to avoid some confusion. Signed-off-by: Jonathan Nieder --- vcs-svn/line_buffer.c | 8 -------- vcs-svn/line_buffer.h | 4 +--- vcs-svn/line_buffer.txt | 12 +++--------- 3 files changed, 4 insertions(+), 20 deletions(-) (limited to 'vcs-svn') diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c index 33e733a04..c39038723 100644 --- a/vcs-svn/line_buffer.c +++ b/vcs-svn/line_buffer.c @@ -91,13 +91,6 @@ char *buffer_read_line(struct line_buffer *buf) return buf->line_buffer; } -char *buffer_read_string(struct line_buffer *buf, uint32_t len) -{ - strbuf_reset(&buf->blob_buffer); - strbuf_fread(&buf->blob_buffer, len, buf->infile); - return ferror(buf->infile) ? NULL : buf->blob_buffer.buf; -} - void buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, uint32_t size) { @@ -134,5 +127,4 @@ off_t buffer_skip_bytes(struct line_buffer *buf, off_t nbytes) void buffer_reset(struct line_buffer *buf) { - strbuf_release(&buf->blob_buffer); } diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h index f5c468afa..d0b22dda7 100644 --- a/vcs-svn/line_buffer.h +++ b/vcs-svn/line_buffer.h @@ -7,10 +7,9 @@ struct line_buffer { char line_buffer[LINE_BUFFER_LEN]; - struct strbuf blob_buffer; FILE *infile; }; -#define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL} +#define LINE_BUFFER_INIT { "", NULL } int buffer_init(struct line_buffer *buf, const char *filename); int buffer_fdinit(struct line_buffer *buf, int fd); @@ -23,7 +22,6 @@ long buffer_tmpfile_prepare_to_read(struct line_buffer *buf); int buffer_ferror(struct line_buffer *buf); char *buffer_read_line(struct line_buffer *buf); -char *buffer_read_string(struct line_buffer *buf, uint32_t len); int buffer_read_char(struct line_buffer *buf); void buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, uint32_t len); /* Returns number of bytes read (not necessarily written). */ diff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt index 4ef0755cf..8e139eb22 100644 --- a/vcs-svn/line_buffer.txt +++ b/vcs-svn/line_buffer.txt @@ -16,8 +16,8 @@ The calling program: - initializes a `struct line_buffer` to LINE_BUFFER_INIT - specifies a file to read with `buffer_init` - - processes input with `buffer_read_line`, `buffer_read_string`, - `buffer_skip_bytes`, and `buffer_copy_bytes` + - processes input with `buffer_read_line`, `buffer_skip_bytes`, + and `buffer_copy_bytes` - closes the file with `buffer_deinit`, perhaps to start over and read another file. @@ -37,7 +37,7 @@ the calling program. A program the temporary file - declares writing is over with `buffer_tmpfile_prepare_to_read` - can re-read what was written with `buffer_read_line`, - `buffer_read_string`, and so on + `buffer_copy_bytes`, and so on - can reuse the temporary file by calling `buffer_tmpfile_rewind` again - removes the temporary file with `buffer_deinit`, perhaps to @@ -64,12 +64,6 @@ Functions Read a line and strip off the trailing newline. On failure or end of file, returns NULL. -`buffer_read_string`:: - Read `len` characters of input or up to the end of the - file, whichever comes first. Returns NULL on error. - Returns whatever characters were read (possibly "") - for end of file. - `buffer_copy_bytes`:: Read `len` bytes of input and dump them to the standard output stream. Returns early for error or end of file. -- cgit v1.2.1 From 4c3169b03ec567ac43edcfc08ffdab119e0ebe94 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 24 Mar 2011 23:10:00 -0500 Subject: vcs-svn: avoid unnecessary copying of log message and author Use strbuf_swap when storing the svn:log and svn:author properties, so pointers to rather than the contents of buffers get copied. The main effect should be to make the code a little easier to read. Signed-off-by: Jonathan Nieder --- vcs-svn/svndump.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'vcs-svn') diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c index c00f03117..88ecef106 100644 --- a/vcs-svn/svndump.c +++ b/vcs-svn/svndump.c @@ -83,7 +83,7 @@ static void reset_dump_ctx(const char *url) } static void handle_property(const struct strbuf *key_buf, - const char *val, uint32_t len, + struct strbuf *val, uint32_t *type_set) { const char *key = key_buf->buf; @@ -95,23 +95,23 @@ static void handle_property(const struct strbuf *key_buf, break; if (!val) die("invalid dump: unsets svn:log"); - strbuf_reset(&rev_ctx.log); - strbuf_add(&rev_ctx.log, val, len); + strbuf_swap(&rev_ctx.log, val); break; case sizeof("svn:author"): if (constcmp(key, "svn:author")) break; - strbuf_reset(&rev_ctx.author); - if (val) - strbuf_add(&rev_ctx.author, val, len); + if (!val) + strbuf_reset(&rev_ctx.author); + else + strbuf_swap(&rev_ctx.author, val); break; case sizeof("svn:date"): if (constcmp(key, "svn:date")) break; if (!val) die("invalid dump: unsets svn:date"); - if (parse_date_basic(val, &rev_ctx.timestamp, NULL)) - warning("invalid timestamp: %s", val); + if (parse_date_basic(val->buf, &rev_ctx.timestamp, NULL)) + warning("invalid timestamp: %s", val->buf); break; case sizeof("svn:executable"): case sizeof("svn:special"): @@ -187,10 +187,10 @@ static void read_props(void) strbuf_swap(&key, &val); continue; case 'D': - handle_property(&val, NULL, 0, &type_set); + handle_property(&val, NULL, &type_set); continue; case 'V': - handle_property(&key, val.buf, len, &type_set); + handle_property(&key, &val, &type_set); strbuf_reset(&key); continue; default: -- cgit v1.2.1 From 195b7ca6f229455da61f9f6b6e56a6558fb0e8ee Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 26 Mar 2011 00:49:37 -0500 Subject: vcs-svn: handle log message with embedded NUL Pass the log message by strbuf instead of as a C-style string and use fwrite instead of printf to write it to fast-import so embedded '\0' bytes can be preserved. Currently "git log" doesn't show the embedded NULs but "git cat-file commit" can. While at it, stop including system headers from repo_tree.h. git source files need to include git-compat-util.h (or cache.h or builtin.h) sooner to ensure the appropriate feature test macros are defined. Signed-off-by: Jonathan Nieder --- vcs-svn/fast_export.c | 12 +++++++----- vcs-svn/fast_export.h | 7 ++++--- vcs-svn/repo_tree.c | 5 +++-- vcs-svn/repo_tree.h | 4 ++-- vcs-svn/svndump.c | 2 +- 5 files changed, 17 insertions(+), 13 deletions(-) (limited to 'vcs-svn') diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c index a4d4d9993..2e5bb6725 100644 --- a/vcs-svn/fast_export.c +++ b/vcs-svn/fast_export.c @@ -31,12 +31,14 @@ void fast_export_modify(uint32_t depth, uint32_t *path, uint32_t mode, } static char gitsvnline[MAX_GITSVN_LINE_LEN]; -void fast_export_commit(uint32_t revision, const char *author, char *log, +void fast_export_commit(uint32_t revision, const char *author, + const struct strbuf *log, const char *uuid, const char *url, unsigned long timestamp) { + static const struct strbuf empty = STRBUF_INIT; if (!log) - log = ""; + log = ∅ if (*uuid && *url) { snprintf(gitsvnline, MAX_GITSVN_LINE_LEN, "\n\ngit-svn-id: %s@%"PRIu32" %s\n", @@ -49,9 +51,9 @@ void fast_export_commit(uint32_t revision, const char *author, char *log, *author ? author : "nobody", *author ? author : "nobody", *uuid ? uuid : "local", timestamp); - printf("data %"PRIu32"\n%s%s\n", - (uint32_t) (strlen(log) + strlen(gitsvnline)), - log, gitsvnline); + printf("data %"PRIuMAX"\n", log->len + strlen(gitsvnline)); + fwrite(log->buf, log->len, 1, stdout); + printf("%s\n", gitsvnline); if (!first_commit_done) { if (revision > 1) printf("from refs/heads/master^0\n"); diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h index 05cf97f3a..33a8fe996 100644 --- a/vcs-svn/fast_export.h +++ b/vcs-svn/fast_export.h @@ -2,13 +2,14 @@ #define FAST_EXPORT_H_ #include "line_buffer.h" +struct strbuf; void fast_export_delete(uint32_t depth, uint32_t *path); void fast_export_modify(uint32_t depth, uint32_t *path, uint32_t mode, uint32_t mark); -void fast_export_commit(uint32_t revision, const char *author, char *log, - const char *uuid, const char *url, - unsigned long timestamp); +void fast_export_commit(uint32_t revision, const char *author, + const struct strbuf *log, const char *uuid, + const char *url, unsigned long timestamp); void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len, struct line_buffer *input); diff --git a/vcs-svn/repo_tree.c b/vcs-svn/repo_tree.c index d722e3212..8caa0159d 100644 --- a/vcs-svn/repo_tree.c +++ b/vcs-svn/repo_tree.c @@ -278,8 +278,9 @@ void repo_diff(uint32_t r1, uint32_t r2) repo_commit_root_dir(commit_pointer(r2))); } -void repo_commit(uint32_t revision, const char *author, char *log, - const char *uuid, const char *url, unsigned long timestamp) +void repo_commit(uint32_t revision, const char *author, + const struct strbuf *log, const char *uuid, const char *url, + unsigned long timestamp) { fast_export_commit(revision, author, log, uuid, url, timestamp); dent_commit(); diff --git a/vcs-svn/repo_tree.h b/vcs-svn/repo_tree.h index a1b0e8765..37bde2e37 100644 --- a/vcs-svn/repo_tree.h +++ b/vcs-svn/repo_tree.h @@ -1,7 +1,7 @@ #ifndef REPO_TREE_H_ #define REPO_TREE_H_ -#include "git-compat-util.h" +struct strbuf; #define REPO_MODE_DIR 0040000 #define REPO_MODE_BLB 0100644 @@ -18,7 +18,7 @@ uint32_t repo_read_path(const uint32_t *path); uint32_t repo_read_mode(const uint32_t *path); void repo_delete(uint32_t *path); void repo_commit(uint32_t revision, const char *author, - char *log, const char *uuid, const char *url, + const struct strbuf *log, const char *uuid, const char *url, long unsigned timestamp); void repo_diff(uint32_t r1, uint32_t r2); void repo_init(void); diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c index 88ecef106..eef49ca19 100644 --- a/vcs-svn/svndump.c +++ b/vcs-svn/svndump.c @@ -274,7 +274,7 @@ static void handle_revision(void) { if (rev_ctx.revision) repo_commit(rev_ctx.revision, rev_ctx.author.buf, - rev_ctx.log.buf, dump_ctx.uuid.buf, dump_ctx.url.buf, + &rev_ctx.log, dump_ctx.uuid.buf, dump_ctx.url.buf, rev_ctx.timestamp); } -- cgit v1.2.1