On Tue, Oct 16, 2012 at 01:34:41PM +0200, René Scharfe wrote:
> FWIW, I couldn't measure a performance difference for git log with and
> without the following patch, which catches commits created with your
> hash collision trick, but might be too strict:
>
> diff --git a/commit.c b/commit.c
> index 213bc98..4cd1e83 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -262,6 +262,12 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
> if (item->object.parsed)
> return 0;
> item->object.parsed = 1;
> +
> + if (memchr(buffer, '\0', size)) {
> + return error("bogus commit contains a NUL character: %s",
> + sha1_to_hex(item->object.sha1));
> + }
> +
Hmm. Yeah, that should be relatively inexpensive, since we are about to
read through most of the bytes anyway (we probably have just zlib
inflated them all, so they would even be in cache). It might make more
of a difference for a raw traversal that is not even going to look at
below the header, like rev-list or merge-base. But I couldn't measure a
difference doing "git rev-list HEAD >/dev/null" in either git.git or
linux-2.6.git.
So maybe it is worth doing preemptively. Even without security concerns,
we would be truncating the commit message, so it is probably better to
let the user know (a warning is probably more appropriate, though, just
in case somebody does have embedded NULs for historical reason).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html