averello/git-mirror
1
0
Fork 0
mirror of https://github.com/git/git.git synced 2025-12-12 20:36:24 +01:00
Code Issues Packages Projects Releases Wiki Activity

upload-pack: don't ACK non-commits repeatedly in protocol v2

Browse Source 
When a client performs a fetch or clone they can optionally send "have"
lines to tell the server which objects they already have available
locally. These object IDs are stored by the server in an object array so
that it can remember any objects it doesn't have to include in the pack
sent to the client.

While there isn't any reason to do so, clients are free to send the same
"have" line repeatedly. git-upload-pack(1) already knows to handle this
well: every commit it has seen via a "have" line gets marked with the
`THEY_HAVE` flag, and if such a commit is seen repeatedly we know to not
process it another time. This also has the effect that we only store the
object ID once, only, in the `have_obj` array.

There is an edge case though: if the client sends an object ID that does
not refer to a commit we neither store nor check the `THEY_HAVE` flag.
This means that we repeatedly store the same object ID in our `have_obj`
array, with two consequences:

  - In protocol v2 we deduplicate ACKs for commits, but not for any
    other objects as we send ACKs for every object ID in the `have_obj`
    array.

  - The `have_obj` array can grow in size indefinitely with both
    protocols.

The potentially-more-serious issue is the second one, as we basically
have a way for an adversary to allocate arbitrarily large buffers now.
Ultimately, this doesn't seem to be all that serious though: on my
machine, the growth of that array is at around 4MB/s, and after roughly
five minutes I was only at 1GB RSS. So this is concerning, but only
mildly so.

Fix this bug by storing the `THEY_HAVE` flag independent of the object
type so that we don't store duplicate object IDs in `have_obj` anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Patrick Steinhardt
2025-09-05 08:18:02 +02:00
committed by Junio C Hamano
parent 7a57fb1a59
commit 88a2dc68c8
2 changed files with 48 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
t/t5530-upload-pack-error.sh
View File

@@ -96,4 +96,43 @@ test_expect_success 'fetch fails' '
test_must_fail git -C foo fetch .. main
'
test_expect_success 'upload-pack ACKs repeated non-commit objects repeatedly (protocol v0)' '
commit_id=$(git rev-parse HEAD) &&
tree_id=$(git rev-parse HEAD^{tree}) &&
test-tool pkt-line pack >request <<-EOF &&
want $commit_id
0000
have $tree_id
have $tree_id
0000
EOF
git upload-pack --stateless-rpc . <request >actual &&
depacketize <actual >actual.raw &&
grep ^ACK actual.raw >actual.acks &&
cat >expect <<-EOF &&
ACK $tree_id
ACK $tree_id
EOF
test_cmp expect actual.acks
'
test_expect_success 'upload-pack ACKs repeated non-commit objects once only (protocol v2)' '
commit_id=$(git rev-parse HEAD) &&
tree_id=$(git rev-parse HEAD^{tree}) &&
test-tool pkt-line pack >request <<-EOF &&
command=fetch
object-format=$(test_oid algo)
0001
want $commit_id
have $tree_id
have $tree_id
0000
EOF
GIT_PROTOCOL=version=2 git upload-pack . <request >actual &&
depacketize <actual >actual.raw &&
grep ^ACK actual.raw >actual.acks &&
echo "ACK $tree_id" >expect &&
test_cmp expect actual.acks
'
test_done

19
upload-pack.c
View File

@@ -476,20 +476,17 @@ static void create_pack_file(struct upload_pack_data *pack_data,
static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)
{
int we_knew_they_have = 0;
struct object *o = parse_object_with_flags(the_repository, oid,
PARSE_OBJECT_SKIP_HASH_CHECK |
PARSE_OBJECT_DISCARD_TREE);
if (!o)
die("oops (%s)", oid_to_hex(oid));
if (o->type == OBJ_COMMIT) {
struct commit_list *parents;
struct commit *commit = (struct commit *)o;
if (o->flags & THEY_HAVE)
we_knew_they_have = 1;
else
o->flags |= THEY_HAVE;
if (!data->oldest_have || (commit->date < data->oldest_have))
data->oldest_have = commit->date;
for (parents = commit->parents;
@@ -497,11 +494,13 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid
parents = parents->next)
parents->item->object.flags |= THEY_HAVE;
}
if (!we_knew_they_have) {
add_object_array(o, NULL, &data->have_obj);
return 1;
}
return 0;
if (o->flags & THEY_HAVE)
return 0;
o->flags |= THEY_HAVE;
add_object_array(o, NULL, &data->have_obj);
return 1;
}
static int got_oid(struct upload_pack_data *data,