git archive broken in 1.7.8.1

CC me on answers since i'm not subscribed to the list

Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax

git archive --remote=git://anongit.kde.org/repo.git HEAD:path

does not seem to return a valid tar archive anymore when it did work previously. In fact the man page of my version has that syntax in one of the examples.

Quoted 5 lines

CC me on answers since i'm not subscribed to the list

Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax

Quoted 1 line

git archive --remote=git://anongit.kde.org/repo.git HEAD:path

This syntax is no longer allowed due to some security tightening. Use the alternate syntax

    git archive --remote=git://anongit.kde.org/repo.git HEAD -- path

Quoted 4 lines

does not seem to return a valid tar archive anymore when it did work previously. In fact the man page of my version has that syntax in one of the examples.

That sounds like a documentation bug.

cmn

El Dimarts, 10 de gener de 2012, a les 22:33:44, Carlos Martín Nieto va escriure:

Quoted 6 lines · Albert Astals Cid

Quoted 5 lines

CC me on answers since i'm not subscribed to the list

Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax

Quoted 6 lines

Quoted 1 line

git archive --remote=git://anongit.kde.org/repo.git HEAD:path

This syntax is no longer allowed due to some security tightening. Use the alternate syntax

    git archive --remote=git://anongit.kde.org/repo.git HEAD -- path

Unfortunately this producess a tarball with a different layout, e.g.

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US
gives me a tarball with the doc/en_US files in the root

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD -- doc/en_US
gives me a tarball with the doc/en_US folders and then the files

Is there a way to keep the old behaviour or do we need to update our scripts?

Thanks for the fast answer!

Albert

Quoted 6 lines

Quoted 4 lines

does not seem to return a valid tar archive anymore when it did work previously. In fact the man page of my version has that syntax in one of the examples.

That sounds like a documentation bug.

Quoted 8 lines

Unfortunately this producess a tarball with a different layout, e.g.

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US
gives me a tarball with the doc/en_US files in the root

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD -- doc/en_US
gives me a tarball with the doc/en_US folders and then the files

Quoted 1 line

Is there a way to keep the old behaviour or do we need to update our scripts?

Not as far as I know. However, the commit that hardened the input (ee27ca4a781844: archive: don't let remote clients get unreachable commits, 2011-11-17) does state that HEAD:doc/en_US should be valid, so it looks like it's actually a regression. As it's bedtime in my timezone, I'm blaming Peff and I'll look into this if it hasn't been fixed by the time I get to the office tomorrow.

The tightening done in (ee27ca4a: archive: don't let remote clients get unreachable commits, 2011-11-17) went too far and disallowed HEAD:Documentation as it would try to find "HEAD:Documentation" as a ref.

Only DWIM the "HEAD" part to see if it exists as a ref. Once we're sure that we've been given a valid ref, we follow the normal code path. This still disallows attempts to access commits which are not branch tips.

AFAICT this should still be safe. Using HEAD^:Documentation or <sha1>:Documentation still complains that HEAD^ and <sha1> aren't refs.

Most callers generally treat get_sha1 as a black box, giving it a string from the user and expecting to get a sha1 in return. The get_sha1_with_context function gives callers more information about what happened while resolving the object name so they can make better decisions about how to use the result. We currently use this only to provide information about the path entry used to find a blob.

We don't currently provide any information about the resolution rules that were used to reach the final object. Some callers may want these in order to enforce a policy that a particular subset of the lookup rules are used (e.g., when serving remote requests).

This patch adds a set of bit-fields that document the use of particular features during an object lookup.

Signed-off-by Jeff King <[email protected]>

---
The diffstat looks a little scary, but it is mostly just the internal get_sha1 functions learning to pass the object_context around.

Quoted 17 lines · Jeff King

Most callers generally treat get_sha1 as a black box, giving it a string from the user and expecting to get a sha1 in return. The get_sha1_with_context function gives callers more information about what happened while resolving the object name so they can make better decisions about how to use the result. We currently use this only to provide information about the path entry used to find a blob.

We don't currently provide any information about the resolution rules that were used to reach the final object. Some callers may want these in order to enforce a policy that a particular subset of the lookup rules are used (e.g., when serving remote requests).

This patch adds a set of bit-fields that document the use of particular features during an object lookup.

Quoted 4 lines

Signed-off-by Jeff King <[email protected]>

---
The diffstat looks a little scary, but it is mostly just the internal get_sha1 functions learning to pass the object_context around.

Hmm, shouldn't this also cover peel_to_type()? That would have made it also apply to the maintenance track.

So, did this patch make it anywhere? We could really use it.

Here's the use case. The original ee27ca4 patch broke our build system when the git server was upgraded to Debian Wheezy last night. The builder fetches source from the repo in two pieces using git archive, and we need to make sure both pieces are from the same commit. So we get a sha1 hash with git ls-remote, and use it with git archive --remote. This, of course, breaks with the 'no such ref' error.

At the very least, the documentation is wrong when it talks about passing a commit ID to git archive: maintainers must surely agree that the documentation and the actual behavior ought to match.

2. Actually do a reachability check. Doing a full object check to allow fetching an arbitrary tree by sha1 is probably prohibitively expensive[2], but we could allow the form "<commit>[:<path>]", check that "<commit>" is reachable, and then allow arbitrary paths within it.

Many code paths will free a tree object's buffer and set it to NULL after finishing with it in order to keep memory usage down during a traversal. However, out of 8 sites that do this, only one actually unsets the "parsed" flag back. Those sites that don't are setting a trap for later users of the tree object; even after calling parse_tree, the buffer will remain NULL, causing potential segfaults.

It is not known whether this is triggerable in the current code. Most commands do not do an in-memory traversal followed by actually using the objects again. However, it does not hurt to be safe for future callers.

In most cases, we can abstract this out to a "free_tree_buffer" helper. However, there are two exceptions:

1. The fsck code relies on the parsed flag to know that we were able to parse the object at one point. We can switch this to using a flag in the "flags" field.

2. The index-pack code sets the buffer to NULL but does not free it (it is freed by a caller). We should still unset the parsed flag here, but we cannot use our helper, as we do not want to free the buffer.

Signed-off-by Jeff King <[email protected]>

---
This shouldn't have any behavior change, but I'd worry a bit that I missed some case in builtin/fsck.c where the new HAS_OBJ flag would need set.

 builtin/fsck.c       | 17 ++++++++---------
 builtin/index-pack.c |  1 +
 builtin/reflog.c     |  3 +--
 http-push.c          |  3 +--
 list-objects.c       |  3 +--
 reachable.c          |  3 +--
 revision.c           |  3 +--
 tree.c               |  8 ++++++++
 tree.h               |  1 +
 walker.c             |  5 +----
 10 files changed, 24 insertions(+), 23 deletions(-)

builtin/fsck.c +8 -9

index bb9a2cd..579fdcc 100644
@@ -16,6 +16,7 @@
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
+#define HAS_OBJ   0x0004
 
 static int show_root;
 static int show_tags;

@@ -101,7 +102,7 @@ static int mark_object(struct object *obj, int type, void *data)
 	if (obj->flags & REACHABLE)
 		return 0;
 	obj->flags |= REACHABLE;
-	if (!obj->parsed) {
+	if (!(obj->flags & HAS_OBJ)) {
 		if (parent && !has_sha1_file(obj->sha1)) {
 			printf("broken link from %7s %s\n",
 				 typename(parent->type), sha1_to_hex(parent->sha1));

@@ -127,16 +128,13 @@ static int traverse_one_object(struct object *obj)
 	struct tree *tree = NULL;
 
 	if (obj->type == OBJ_TREE) {
-		obj->parsed = 0;
 		tree = (struct tree *)obj;
 		if (parse_tree(tree) < 0)
 			return 1; /* error already displayed */
 	}
 	result = fsck_walk(obj, mark_object, obj);
-	if (tree) {
-		free(tree->buffer);
-		tree->buffer = NULL;
-	}
+	if (tree)
+		free_tree_buffer(tree);
 	return result;
 }
 

@@ -178,7 +176,7 @@ static void check_reachable_object(struct object *obj)
 	 * except if it was in a pack-file and we didn't
 	 * do a full fsck
 	 */
-	if (!obj->parsed) {
+	if (!(obj->flags & HAS_OBJ)) {
 		if (has_sha1_pack(obj->sha1))
 			return; /* it is in pack - forget about it */
 		printf("missing %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1));

@@ -306,8 +304,7 @@ static int fsck_obj(struct object *obj)
 	if (obj->type == OBJ_TREE) {
 		struct tree *item = (struct tree *) obj;
 
-		free(item->buffer);
-		item->buffer = NULL;
+		free_tree_buffer(item);
 	}
 
 	if (obj->type == OBJ_COMMIT) {

@@ -340,6 +337,7 @@ static int fsck_sha1(const unsigned char *sha1)
 		return error("%s: object corrupt or missing",
 			     sha1_to_hex(sha1));
 	}
+	obj->flags |= HAS_OBJ;
 	return fsck_obj(obj);
 }
 

@@ -352,6 +350,7 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
 		errors_found |= ERROR_OBJECT;
 		return error("%s: object corrupt or missing", sha1_to_hex(sha1));
 	}
+	obj->flags = HAS_OBJ;
 	return fsck_obj(obj);
 }
 

builtin/index-pack.c +1 -0

index 79dfe47..20cf284 100644
@@ -765,6 +765,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
 			if (obj->type == OBJ_TREE) {
 				struct tree *item = (struct tree *) obj;
 				item->buffer = NULL;
+				obj->parsed = 0;
 			}
 			if (obj->type == OBJ_COMMIT) {
 				struct commit *commit = (struct commit *) obj;

builtin/reflog.c +1 -2

index 54184b3..ba27f7c 100644
@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)
 			complete = 0;
 		}
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 
 	if (complete)
 		tree->object.flags |= SEEN;

http-push.c +1 -2

index 395a8cf..c13b441 100644
@@ -1330,8 +1330,7 @@ static struct object_list **process_tree(struct tree *tree,
 			break;
 		}
 
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 	return p;
 }
 

list-objects.c +1 -2

index 3dd4a96..c8c3463 100644
@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,
 				     cb_data);
 	}
 	strbuf_setlen(base, baselen);
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 static void mark_edge_parents_uninteresting(struct commit *commit,

reachable.c +1 -2

index e7e6a1e..654a8c5 100644
@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,
 		else
 			process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 static void process_tag(struct tag *tag, struct object_array *p,

revision.c +1 -2

index 518cd08..eb988ee 100644
@@ -135,8 +135,7 @@ void mark_tree_uninteresting(struct tree *tree)
 	 * We don't care about the tree any more
 	 * after it has been marked uninteresting.
 	 */
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 void mark_parents_uninteresting(struct commit *commit)

tree.c +8 -0

index 62fed63..1cbf60e 100644
@@ -225,6 +225,14 @@ int parse_tree(struct tree *item)
 	return parse_tree_buffer(item, buffer, size);
 }
 
+void free_tree_buffer(struct tree *tree)
+{
+	free(tree->buffer);
+	tree->buffer = NULL;
+	tree->size = 0;
+	tree->object.parsed = 0;
+}
+
 struct tree *parse_tree_indirect(const unsigned char *sha1)
 {
 	struct object *obj = parse_object(sha1);

tree.h +1 -0

index 69bcb5e..601ab9c 100644
@@ -16,6 +16,7 @@ int parse_tree(struct tree *tree);
 int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
 
 int parse_tree(struct tree *tree);
+void free_tree_buffer(struct tree *tree);
 
 /* Parses and returns the tree in the given ent, chasing tags and commits. */
 struct tree *parse_tree_indirect(const unsigned char *sha1);

walker.c +1 -4

index be389dc..633596e 100644
@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)
 		if (!obj || process(walker, obj))
 			return -1;
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
-	tree->size = 0;
-	tree->object.parsed = 0;
+	free_tree_buffer(tree);
 	return 0;
 }

We cannot create an archive from a blob object, so we would not expect anyone to provide one to us. And if they do, we will fail anyway just after the reachability check. We can therefore optimize our reachability check to ignore blobs completely, and not even create a "struct blob" for them.

Depending on the repository size and the exact place we find the reachable object in the traversal, this can save 20-25%, a we can avoid many lookups in the object hash.

The downside of this is that a blob provided to a remote archive process will fail with "no such object" rather than "object is not a tree" (we could organize the code to retain the old message, but since we no longer know whether the blob is reachable or not, we would potentially be leaking information about the existence of unreachable objects).

We cannot create an archive from a blob object, so we would not expect anyone to provide one to us. And if they do, we will fail anyway just after the reachability check. We can therefore optimize our reachability check to ignore blobs completely, and not even create a "struct blob" for them.

Depending on the repository size and the exact place we find the reachable object in the traversal, this can save 20-25%, a we can avoid many lookups in the object hash.

The downside of this is that a blob provided to a remote archive process will fail with "no such object" rather than "object is not a tree" (we could organize the code to retain the old message, but since we no longer know whether the blob is reachable or not, we would potentially be leaking information about the existence of unreachable objects).

Signed-off-by Jeff King <[email protected]>

Quoted 5 lines · Jeff King

2. Actually do a reachability check. Doing a full object check to allow fetching an arbitrary tree by sha1 is probably prohibitively expensive[2], but we could allow the form "<commit>[:<path>]", check that "<commit>" is reachable, and then allow arbitrary paths within it.

Thinking on this more, the full reachability check is no worse than what a clone has to do to fetch the full repository. Here's a series that does the full check. I'm not entirely happy with the performance, though; details are in patch 3.

I think I'd be tempted to just go the more limiting "commit is reachable" route, instead, which would solve your case (and most sane cases).

[1/4]: clear parsed flag when we free tree buffers
[2/4]: upload-archive: restrict remote objects with reachability check [3/4]: list-objects: optimize "revs->blob_objects = 0" case [4/4]: archive: ignore blob objects when checking reachability

-Peff

The tightening done in (ee27ca4a: archive: don't let remote clients get unreachable commits, 2011-11-17) went too far and disallowed HEAD:Documentation as it would try to find "HEAD:Documentation" as a ref.

Only DWIM the "HEAD" part to see if it exists as a ref. Once we're sure that we've been given a valid ref, we follow the normal code path. This still disallows attempts to access commits which are not branch tips.

Signed-off-by Carlos Martín Nieto <[email protected]>

---
AFAICT this should still be safe. Using HEAD^:Documentation or <sha1>:Documentation still complains that HEAD^ and <sha1> aren't refs.

Quoted 4 lines · Carlos Martín Nieto

The tightening done in (ee27ca4a: archive: don't let remote clients get unreachable commits, 2011-11-17) went too far and disallowed HEAD:Documentation as it would try to find "HEAD:Documentation" as a ref.

I do not think it went too far. Actually we discussed this exact issue when the topic was cooking, and saw no objections. The commit in question itself advertises this restriction.

Why are we loosening it now? I do not see a compelling reason to do so.

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · Carlos Martín Nieto

The tightening done in (ee27ca4a: archive: don't let remote clients get unreachable commits, 2011-11-17) went too far and disallowed HEAD:Documentation as it would try to find "HEAD:Documentation" as a ref.

Quoted 3 lines

I do not think it went too far. Actually we discussed this exact issue when the topic was cooking, and saw no objections. The commit in question itself advertises this restriction.

I think you and I discussed it off list (I originally took this off-list because the original issue did have some security implications). So I don't think people necessarily had a chance to object.

Quoted 9 lines

* jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit
- archive: don't let remote clients get unreachable commits (this branch is used by jk/maint-upload-archive.)

* jk/maint-upload-archive (2011-11-21) 1 commit
- Merge branch 'jk/maint-1.6.2-upload-archive' into jk/maint-upload-archive
(this branch uses jk/maint-1.6.2-upload-archive.)

Quoted 1 line

Will merge to 'next' after taking another look.

Thanks. I also have some followup patches to re-loosen to at least trees reachable from refs. Do you want to leave the tightening to the maint track, and then consider the re-loosening for master?

I see it the opposite way. People are clearly using the "$ref:$path" syntax. So why would we restrict them from doing so? There are no security implications (i.e., they could always just grab $ref and extract $path themselves). In my view, ee27ca4a was over-eager in its restrictions because I wanted it to be simple and close the hole. Now we can take our time adding more code to loosen it.

Quoted 6 lines · Jeff King

I see it the opposite way. People are clearly using the "$ref:$path" syntax. So why would we restrict them from doing so? There are no security implications (i.e., they could always just grab $ref and extract $path themselves). In my view, ee27ca4a was over-eager in its restrictions because I wanted it to be simple and close the hole. Now we can take our time adding more code to loosen it.

Ok, so it is more like a partial revert of whatever we did. In that case, I'd take CMN's patch to limit the extent of the changes, as it more closely matches the spirit of the original ee27ca4 (archive: don't let remote clients get unreachable commits, 2011-11-17) that singled out and catered to the need of "archive" command alone. It is already part of the v1.7.8.1 release, so I would prefer a change to be stupid and simple.

For a maint release, I am OK with that. In the long term, I'd rather my patches go onto master (either for 1.7.9 or for later), as I think they are the right way to do it.

Unfortunately this producess a tarball with a different layout, e.g.

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US
gives me a tarball with the doc/en_US files in the root

Quoted 4 lines · Albert Astals Cid

Unfortunately this producess a tarball with a different layout, e.g.

git archive --remote=git://anongit.kde.org/kgraphviewer.git HEAD:doc/en_US
gives me a tarball with the doc/en_US files in the root

Meaning the files you have stored in git under doc/en_US are dumped in the root directory of the tar? That does not sound like desired behavior for the feature.