From a6eec1263883ce9787a354e1635b7b732e72c3c9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 16 Mar 2013 06:25:25 -0400 Subject: upload-pack: drop lookup-before-parse optimization When we receive a "have" line from the client, we want to load the object pointed to by the sha1. However, we are careful to do: o = lookup_object(sha1); if (!o || !o->parsed) o = parse_object(sha1); to avoid loading the object from disk if we have already seen it. However, since ccdc603 (parse_object: try internal cache before reading object db), parse_object already does this optimization internally. We can just call parse_object directly. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- upload-pack.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index 6142421ea1..e29d5d2085 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -327,9 +327,7 @@ static int got_sha1(char *hex, unsigned char *sha1) if (!has_sha1_file(sha1)) return -1; - o = lookup_object(sha1); - if (!(o && o->parsed)) - o = parse_object(sha1); + o = parse_object(sha1); if (!o) die("oops (%s)", sha1_to_hex(sha1)); if (o->type == OBJ_COMMIT) { -- cgit v1.2.3 From 06f15bf1f340f37293b471d8556842d4e89ff63f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 16 Mar 2013 06:27:01 -0400 Subject: upload-pack: make sure "want" objects are parsed When upload-pack receives a "want" line from the client, it adds it to an object array. We call lookup_object to find the actual object, which will only check for objects already in memory. This works because we are expecting to find objects that we already loaded during the ref advertisement. We use the resulting object structs for a variety of purposes. Some of them care only about the object flags, but others care about the type of the object (e.g., ok_to_give_up), or even feed them to the revision parser (when --depth is used), which assumes that objects it receives are fully parsed. Once upon a time, this was OK; any object we loaded into memory would also have been parsed. But since 435c833 (upload-pack: use peel_ref for ref advertisements, 2012-10-04), we try to avoid parsing objects during the ref advertisement. This means that lookup_object may return an object with a type of OBJ_NONE. The resulting mess depends on the exact set of objects, but can include the revision parser barfing, or the shallow code sending the wrong set of objects. This patch teaches upload-pack to parse each "want" object as we receive it. We do not replace the lookup_object call with parse_object, as the current code is careful not to let just any object appear on a "want" line, but rather only one we have previously advertised (whereas parse_object would actually load any arbitrary object from disk). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- upload-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index e29d5d2085..b6ec605b09 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -639,7 +639,7 @@ static void receive_needs(void) use_include_tag = 1; o = lookup_object(sha1_buf); - if (!o) + if (!o || !parse_object(o->sha1)) die("git upload-pack: not our ref %s", sha1_to_hex(sha1_buf)); if (!(o->flags & WANTED)) { -- cgit v1.2.3 From f59de5d1ff9b0f9d570df99128f41520a281f9a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 16 Mar 2013 06:28:30 -0400 Subject: upload-pack: load non-tip "want" objects from disk It is a long-time security feature that upload-pack will not serve any "want" lines that do not correspond to the tip of one of our refs. Traditionally, this was enforced by checking the objects in the in-memory hash; they should have been loaded and received the OUR_REF flag during the advertisement. The stateless-rpc mode, however, has a race condition here: one process advertises, and another receives the want lines, so the refs may have changed in the interim. To address this, commit 051e400 added a new verification mode; if the object is not OUR_REF, we set a "has_non_tip" flag, and then later verify that the requested objects are reachable from our current tips. However, we still die immediately when the object is not in our in-memory hash, and at this point we should only have loaded our tip objects. So the check_non_tip code path does not ever actually trigger, as any non-tip objects would have already caused us to die. We can fix that by using parse_object instead of lookup_object, which will load the object from disk if it has not already been loaded. We still need to check that parse_object does not return NULL, though, as it is possible we do not have the object at all. A more appropriate error message would be "no such object" rather than "not our ref"; however, we do not want to leak information about what objects are or are not in the object database, so we continue to use the same "not our ref" message that would be produced by an unreachable object. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- upload-pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index b6ec605b09..6152a98757 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -638,8 +638,8 @@ static void receive_needs(void) if (parse_feature_request(features, "include-tag")) use_include_tag = 1; - o = lookup_object(sha1_buf); - if (!o || !parse_object(o->sha1)) + o = parse_object(sha1_buf); + if (!o) die("git upload-pack: not our ref %s", sha1_to_hex(sha1_buf)); if (!(o->flags & WANTED)) { -- cgit v1.2.3