[PATCH] receive-pack, fetch-pack: reject bogus pack that records objects twice

When receive-pack & fetch-pack are run and store the pack obtained over
the wire to a local repository, they internally run the index-pack command
with the --strict option. Make sure that we reject incoming packfile that
records objects twice to avoid spreading such a damage.

Signed-off-by: Junio C Hamano <redacted>
---
 * Passing --strict from fetch-pack actually is a recent invention, so
   this will be only useful to 1.7.8 and later.

 builtin/index-pack.c |    4 +++-
 object.c             |    2 ++
 pack-write.c         |    4 ++++
 pack.h               |    3 ++-
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0945adb..98025da 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -1122,8 +1122,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 		if (!index_name)
 			die("--verify with no packfile name given");
 		read_idx_option(&opts, index_name);
-		opts.flags |= WRITE_IDX_VERIFY;
+		opts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;
 	}
+	if (strict)
+		opts.flags |= WRITE_IDX_STRICT;
 
 	curr_pack = open_pack_file(pack_name);
 	parse_pack_header();

diff --git a/object.c b/object.c
index 31976b5..d8d09f9 100644
--- a/object.c
+++ b/object.c

@@ -149,6 +149,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 		struct tree *tree = lookup_tree(sha1);
 		if (tree) {
 			obj = &tree->object;
+			if (!tree->buffer)
+				tree->object.parsed = 0;
 			if (!tree->object.parsed) {
 				if (parse_tree_buffer(tree, buffer, size))
 					return NULL;

diff --git a/pack-write.c b/pack-write.c
index 9cd3bfb..f84adde 100644
--- a/pack-write.c
+++ b/pack-write.c

@@ -129,6 +129,10 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		}
 		sha1write(f, obj->sha1, 20);
 		git_SHA1_Update(&ctx, obj->sha1, 20);
+		if ((opts->flags & WRITE_IDX_STRICT) &&
+		    (i && !hashcmp(list[-2]->sha1, obj->sha1)))
+			die("The same object %s appears twice in the pack",
+			    sha1_to_hex(obj->sha1));
 	}
 
 	if (index_version >= 2) {

diff --git a/pack.h b/pack.h
index 722a54e..aca4739 100644
--- a/pack.h
+++ b/pack.h

@@ -37,7 +37,8 @@ struct pack_header {
 struct pack_idx_option {
 	unsigned flags;
 	/* flag bits */
-#define WRITE_IDX_VERIFY 01
+#define WRITE_IDX_VERIFY 01 /* verify only, do not write the idx file */
+#define WRITE_IDX_STRICT 02
 
 	uint32_t version;
 	uint32_t off32_limit;

-- 
1.7.8.rc2.109.g72037

On Wed, Nov 16, 2011 at 22:04, Junio C Hamano [off-list ref] wrote:

When receive-pack & fetch-pack are run and store the pack obtained over
the wire to a local repository, they internally run the index-pack command
with the --strict option. Make sure that we reject incoming packfile that
records objects twice to avoid spreading such a damage.

Signed-off-by: Junio C Hamano <redacted>

Looks sane to me. I thought this would be more difficult to catch and
report to the user. :-)

On Wed, Nov 16, 2011 at 10:04:13PM -0800, Junio C Hamano wrote:

When receive-pack & fetch-pack are run and store the pack obtained over
the wire to a local repository, they internally run the index-pack command
with the --strict option. Make sure that we reject incoming packfile that
records objects twice to avoid spreading such a damage.

If we are fixing a thin pack (which should be the case most of the
time), we are rewriting the packfile anyway. Shouldn't we just omit
the duplicate?

I guess I'm a little confused about what is generating these duplicates.
A buggy git? A malicious server? Bad luck?

-Peff

On Fri, Nov 18, 2011 at 02:33, Jeff King [off-list ref] wrote:

On Wed, Nov 16, 2011 at 10:04:13PM -0800, Junio C Hamano wrote:

quoted

When receive-pack & fetch-pack are run and store the pack obtained over
the wire to a local repository, they internally run the index-pack command
with the --strict option. Make sure that we reject incoming packfile that
records objects twice to avoid spreading such a damage.

If we are fixing a thin pack (which should be the case most of the
time), we are rewriting the packfile anyway. Shouldn't we just omit
the duplicate?

I guess I'm a little confused about what is generating these duplicates.
A buggy git? A malicious server? Bad luck?

A buggy Git. We found a case where JGit could generate duplicate
objects in the pack stream during a clone. The resulting client
worked... until it tried to do `git gc` or really any sort of `git
pack-objects`.

In my opinion, a pack should never contain duplicate objects. Its a
buggy remote that sends them. What I like about this patch is it stops
and tells the user the remote is broken, which it is.