Protocol v2: The wrong --shallow-since time format causes git to wait indefinitely

[fcharlie]

Recently, when I tested my Git Over HTTP server, I found a bug in Git. When we used git -c protocol.version=2 clone --shallow-since , if the time format is wrong, it will cause git to wait indefinitely.

Github supports Wire Protocol to reproduce, Gitlab does not support it and does not reproduce it.

The commands that can be retried are as follows:

# time format wrong
git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
# trace
GIT_TRACE=1 GIT_CURL_VERBOSE=1 GIT_TRACE_PACKET=2 git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012

We can see that the server closes the HTTP connection after sending the shallow-info\n message, and git-remote-http(s) is still waiting.

$ git --version --build-options
git version 2.24.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8

When the time format is correct, the clone is successful, so this is followed by the shallow-list：

GIT_TRACE=1 GIT_CURL_VERBOSE=1 GIT_TRACE_PACKET=2 git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=2015-10-12

[dscho]

@fcharlie good find. Would you terribly mind reporting this to the Git mailing list at git@vger.kernel.org (no HTML mails, please, they will bounce)?

[fcharlie]

@dscho Thanks. When I switched Outlook to plain text format, it seems to have been successfully sent.

[Denton-L]

Hi @fcharlie, I looked on public-inbox but I couldn't find the thread you made. Would you mind resending the report please?

Thanks!

[fcharlie]

@Denton-L OK.

[fcharlie]

@Denton-L Sorry, maillist rejected my email (I have switched outlook to plain text.)

Hello [*], for your MAIL FROM address <*@outlook.com> policy analysis reported: Your address is not liked source for email

UPDATE I sent it successfully using another email address.

[dscho]

I sent it successfully using another email address.

@fcharlie that is not okay. We pride ourselves in our openness by choosing such an accessible medium as emails for collaboration, and then we just lock out a large swath of contributors, just like that. That makes me very sad.

[Denton-L]

Thread here: https://public-inbox.org/git/tencent_AB1A7466D1F2EF9A84B437705F06A7652709@qq.com/#t

[Denton-L]

Peff's analysis of the issue here:

On Sat, Mar 28, 2020 at 11:13:00AM -0400, Jeff King wrote:

On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:

But since it works for v1 (which also dies!), and since you were able to
reproduce the problem locally, I suspect it may be an issue in
upload-pack itself.

Actually, I think the problem is on the client side.

If I run your test without the http-backend change, then nothing is left
running on the server side, but on the client we have two processes:
git-clone and the git-remote-https helper. And they're deadlocked trying
to read from each other.

I think the issue is that git-remote-https in v2 mode calls into
stateless_connect(), and just pumps http request/response pairs from
git-clone to the server. But if a request generates an empty response,
then clone has no idea that anything was received at all. It continues
waiting, but remote-https has looped to expect another request from it.

Your patch fixes this case because it causes the server to send a
non-empty response. But the client can't rely on that. Besides that not
being a documented server behavior, in the worst case the connection
could be severed mid-stream. So I think remote-https needs to complain
about an empty response. This isn't a problem in v1 because it would
actually try to look at that empty response; in v2 it's just proxying
bytes around.

Ugh, it's actually much worse than this. We dealt with the
empty-response case in 296b847 (remote-curl: don't hang when a server
dies before any output, 2016-11-18). The issue here is that we got a
partial response, and clone is waiting for the rest of it.

The server said "0011shallow-info\n" before it bailed. So from the
perspective of git-clone, it's now waiting to read through a flush
packet. But remote-https has nothing left to send. The root of the issue
is that it has no way to signal to clone that it has already sent
everything the server gave it. In non-helper code, clone would see the
EOF. And in v1 https, I think there's some extra framing between
remote-https and "fetch-pack --stateless-rpc" so that EOF effectively
gets passed along. But v2's stateless_connect() strategy is
fundamentally missing this signal, and there are probably other spots in
the protocol that could cause similar deadlocks.

I wonder if remote-https's stateless_connect() could complain if there's
no flush packet in the output it writes back. That would certainly fix
this case, but I'd worry there are rpc messages that don't end in a
flush. And it would be susceptible to data cut-offs that happened to
look like a flush packet.

I think the robust solution is to introduce an extra level of pktline
framing into the remote-helper stateless-connect protocol.

-Peff

[fcharlie]

Peff's analysis of the issue here:

On Sat, Mar 28, 2020 at 11:13:00AM -0400, Jeff King wrote:

On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:

But since it works for v1 (which also dies!), and since you were able to
reproduce the problem locally, I suspect it may be an issue in
upload-pack itself.

Actually, I think the problem is on the client side.
If I run your test without the http-backend change, then nothing is left
running on the server side, but on the client we have two processes:
git-clone and the git-remote-https helper. And they're deadlocked trying
to read from each other.
I think the issue is that git-remote-https in v2 mode calls into
stateless_connect(), and just pumps http request/response pairs from
git-clone to the server. But if a request generates an empty response,
then clone has no idea that anything was received at all. It continues
waiting, but remote-https has looped to expect another request from it.
Your patch fixes this case because it causes the server to send a
non-empty response. But the client can't rely on that. Besides that not
being a documented server behavior, in the worst case the connection
could be severed mid-stream. So I think remote-https needs to complain
about an empty response. This isn't a problem in v1 because it would
actually try to look at that empty response; in v2 it's just proxying
bytes around.

Ugh, it's actually much worse than this. We dealt with the
empty-response case in 296b847 (remote-curl: don't hang when a server
dies before any output, 2016-11-18). The issue here is that we got a
partial response, and clone is waiting for the rest of it.
The server said "0011shallow-info\n" before it bailed. So from the
perspective of git-clone, it's now waiting to read through a flush
packet. But remote-https has nothing left to send. The root of the issue
is that it has no way to signal to clone that it has already sent
everything the server gave it. In non-helper code, clone would see the
EOF. And in v1 https, I think there's some extra framing between
remote-https and "fetch-pack --stateless-rpc" so that EOF effectively
gets passed along. But v2's stateless_connect() strategy is
fundamentally missing this signal, and there are probably other spots in
the protocol that could cause similar deadlocks.
I wonder if remote-https's stateless_connect() could complain if there's
no flush packet in the output it writes back. That would certainly fix
this case, but I'd worry there are rpc messages that don't end in a
flush. And it would be susceptible to data cut-offs that happened to
look like a flush packet.
I think the robust solution is to introduce an extra level of pktline
framing into the remote-helper stateless-connect protocol.
-Peff

Very good, finally saw that this problem will be solved.

[Denton-L]

Closed by b0df0c1. Thanks for your patience :)