NotificationsYou must be signed in to change notification settings
Fork2.5k
Star10k

smart: ignore shallow/unshallow packets during ACK processing#6973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ethomson merged 1 commit intolibgit2:mainfromkempniu:ignore-shallow-packets-during-ack-processing

Dec 23, 2024

Merged

smart: ignore shallow/unshallow packets during ACK processing#6973

ethomson merged 1 commit intolibgit2:mainfromkempniu:ignore-shallow-packets-during-ack-processing

Dec 23, 2024

Conversation

Copy link

Contributor

kempniu commentedDec 22, 2024•
edited
Loading

In RPC mode (https), the client sends its list of shallow commits at the beginning of each request during packfile negotiation, so that the remote endpoint keeps context. This causes the remote to prepend appropriate shallow/unshallow packets to each response sent back to the client.

However, the store_common() helper function (used in multi_ack mode) does not cater for this, returning as soon as it encounters any packet different than an ACK packet and therefore leaving the rest of the HTTP buffer unprocessed. This in turn causes subsequent iterations of the while loop processing ACK packets to process data returned by older HTTP requests instead of the current one, messing up the packfile negotiation process. Given that the wait_while_ack() helper function (called after the client signals to the remote that it is ready to receive packfile data) correctly skips over shallow/unshallow packets, packfile contents can still be received successfully in some cases (depending on message framing); in some other ones, though (particularly when git_smart__download_pack() processes an HTTP buffer starting with shallow/unshallow packets), the fetching process fails with an "incomplete pack header" error due to the flush packet terminating a set of shallow/unshallow packets being incorrectly interpreted as the flush packet indicating the end of the packfile (making the code behave as if no packfile data was sent by the remote).

Fix by ignoring shallow/unshallow packets in the store_common() helper function, therefore making the ACK processing logic work on the correct HTTP buffers and ensuring that git_smart__download_pack() is not called until packfile negotiation is actually finished.

This bug was fairly nasty to pinpoint, so it was also challenging to come up with a test triggering it, particularly given that the test repository used intests/libgit2/online/shallow.c only contains 21 commits, i.e. barely enough to exceed the 20-packethave line split point. In fact, the test included in this PR only demonstrates a case in which the code is able to (mostly accidentally) "resync" the packfile negotiation process and fetch packfile data after all:

$ ./libgit2_tests -t -sonline::shallow::deepen_fullTAP version 13# start of suite 1: online::shallowok 1 - online::shallow::deepen_full1..1

To make this test trigger theincomplete pack header error using the test repository, an extra modification lowering the maximum count ofhave lines per request must be applied (which does not affect the way the ACK processing logic works on a higher level), e.g.:

diff --git a/src/libgit2/transports/smart_protocol.c b/src/libgit2/transports/smart_protocol.cindex 87c190458..7280bd3fc 100644--- a/src/libgit2/transports/smart_protocol.c+++ b/src/libgit2/transports/smart_protocol.c@@ -488,7 +481,7 @@ int git_smart__negotiate_fetch(  git_pkt_buffer_have(&oid, &data); i++;-if (i % 20 == 0) {+if (i % 5 == 0) { if (t->cancelled.val) { git_error_set(GIT_ERROR_NET, "The fetch was cancelled by the user"); error = GIT_EUSER;@@ -527,7 +520,7 @@ int git_smart__negotiate_fetch( if (t->common.length > 0) break;-if (i % 20 == 0 && t->rpc) {+if (i % 5 == 0 && t->rpc) { git_pkt_ack *pkt; unsigned int j;

With the above modification in place, the test fails:

$ ./libgit2_tests -t -sonline::shallow::deepen_fullTAP version 13# start of suite 1: online::shallownot ok 1 - online::shallow::deepen_full    ---    reason: |      Function call failed: (git_remote_fetch(origin, ((void *)0), &fetch_opts, ((void *)0)))      error -1 - incomplete pack header    at:      file: '/tmp/libgit2/tests/libgit2/online/shallow.c'      line: 191      function: 'test_online_shallow__deepen_full'    ---1..1

Adding HTTP tracing to the test code hopefully further clarifies what happens behind the scenes, e.g.:

diff --git a/tests/libgit2/online/shallow.c b/tests/libgit2/online/shallow.cindex a5508c16d..16b7d58c2 100644--- a/tests/libgit2/online/shallow.c+++ b/tests/libgit2/online/shallow.c@@ -165,6 +165,11 @@ void test_online_shallow__unshallow(void)        git_repository_free(repo); }+static void print_to_stderr(git_trace_level_t level, const char *message)+{+        fprintf(stderr, "[%d] %s\n", level, message);+}+ void test_online_shallow__deepen_full(void) {        git_str path = GIT_STR_INIT;@@ -179,6 +184,8 @@ void test_online_shallow__deepen_full(void)        size_t num_commits = 0;        int error = 0;+       git_trace_set(GIT_TRACE_TRACE, print_to_stderr);+        clone_opts.fetch_opts.depth = 7;        clone_opts.remote_cb = remote_single_branch;

With these two modifications applied (i % 5 and tracing), the test triggers the following behaviorwithout the fix proposed in this PR:

$ ./libgit2_tests -t -sonline::shallow::deepen_full 2>&1 | grep -E -e "Sending (GET|POST)" -e "Received"[5] Sending GET request to https://github.com/libgit2/TestGitRepository/info/refs?service=git-upload-pack[6] Received:[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[5] Sending GET request to https://github.com/libgit2/TestGitRepository/info/refs?service=git-upload-pack[6] Received:[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:$ echo ${PIPESTATUS[0]}1

Note in particular how the code does not read new data from the wire between some of the later POST requests - that's the "out of sync" HTTP buffer processing in action.

With the fix proposed in this PR applied, the above becomes:

$ ./libgit2_tests -t -sonline::shallow::deepen_full 2>&1 | grep -E -e "Sending (GET|POST)" -e "Received"[5] Sending GET request to https://github.com/libgit2/TestGitRepository/info/refs?service=git-upload-pack[6] Received:[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[6] Received:[5] Sending GET request to https://github.com/libgit2/TestGitRepository/info/refs?service=git-upload-pack[6] Received:[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:[5] Sending POST request to https://github.com/libgit2/TestGitRepository/git-upload-pack[6] Received:$ echo ${PIPESTATUS[0]}0

Looking at the code, I believe a similar issue affects non-multi_ack packfile negotiation (I would expect theunexpected pkt type error to be raised), but I did not have the heart to test and/or fix against a server thatdoes support shallow clones whilenot supporting multi_ack mode...

smart: ignore shallow/unshallow packets during ACK processing

bed00cd

In RPC mode (https), the client sends its list of shallow commits at thebeginning of each request during packfile negotiation, so that theremote endpoint keeps context.  This causes the remote to prependappropriate shallow/unshallow packets to each response sent back to theclient.However, the store_common() helper function (used in multi_ack mode)does not cater for this, returning as soon as it encounters any packetdifferent than an ACK packet and therefore leaving the rest of the HTTPbuffer unprocessed.  This in turn causes subsequent iterations of thewhile loop processing ACK packets to process data returned by older HTTPrequests instead of the current one, messing up the packfile negotiationprocess.  Given that the wait_while_ack() helper function (called afterthe client signals to the remote that it is ready to receive packfiledata) correctly skips over shallow/unshallow packets, packfile contentscan still be received successfully in some cases (depending on messageframing); in some other ones, though (particularly whengit_smart__download_pack() processes an HTTP buffer starting withshallow/unshallow packets), the fetching process fails with an"incomplete pack header" error due to the flush packet terminating a setof shallow/unshallow packets being incorrectly interpreted as the flushpacket indicating the end of the packfile (making the code behave as ifno packfile data was sent by the remote).Fix by ignoring shallow/unshallow packets in the store_common() helperfunction, therefore making the ACK processing logic work on the correctHTTP buffers and ensuring that git_smart__download_pack() is not calleduntil packfile negotiation is actually finished.