From: Jeff King <hidden> Date: 2016-06-15 22:56:24
This series fixes the issue I mentioned recently with upload-pack, where
we might feed unparsed objects to the revision parser. The bug is in
435c833 (the tip of the jk/peel-ref topic), which is in v1.8.1 and up.
The fix should go to maint. The bug breaks shallow clones from
repositories with packed refs, though the effects range from working OK
to sending too many objects to calling die() in the revision traversal
machinery. Given the size of the breakage (I had several bug reports
within a few hours of deploying v1.8.1.5 on github.com), and the fact
that the bug was introduced only in v1.8.1, it may make sense to roll a
v1.8.1.6 in addition to putting it on the 1.8.2 maint track.
[1/3]: upload-pack: drop lookup-before-parse optimization
This is just a cleanup, and is not necessary for the fix.
[2/3]: upload-pack: make sure "want" objects are parsed
This is the fix itself, which can stand alone from the other two
patches, both semantically and textually.
[3/3]: upload-pack: load non-tip "want" objects from disk
While investigating the bug, I found some weirdness around the
stateless-rpc check_non_tip code. As far as I can tell, that code
never actually gets triggered. It's not too surprising that we
wouldn't have noticed, because it is about falling back due to a
race condition. But please sanity check my explanation and patch.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:25
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 <redacted>
---
upload-pack.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:25
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 <redacted>
---
t/t5500-fetch-pack.sh | 9 +++++++++
upload-pack.c | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:25
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 <redacted>
---
upload-pack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)