Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects

On Mon, May 18, 2020 at 01:36:52PM -0400, Denton Liu wrote:

Yeah, I think we should avoid using any packet that could be confused with regular output. A well-functioning server should have sent a flush already, and so the worst case would probably be that we would mistake their "0028" packet for ours if the network happens to cut out right at that moment (and even that is obviously exceedingly unlikely). But it just seems easier to reason about if we know we can't get confused.

Yuck. :) I think v0 does it by unwrapping the extra layer in fetch-pack, but I'd have to double check. But I'm not going to, because I think we both agree that your other approaches are way nicer.

Yeah, agreed that the efficacy of the "must end in flush" strategy depends on there not being internal flushes. At least against a determined attacker. But it may also be less random than you might think, if the pattern is to send a flush, then do a bunch of work, etc. A "random" hangup or error might be more likely to happen at the flush points then (from the client's perspective).

So let's see if we can answer that question with less hand-waving.

The outer v2 capabilities conversation only writes out the capabilities list, followed by a single flush (the packet_flush() call in advertise_capabilities()). So far so good.

The only two v2 commands defined in serve.c are "ls-refs" and "fetch".

For "ls-refs", we end up in ls_refs(). That writes only regular packets via send_ref(), and then concludes with a single flush (we don't even send a delim; the client does that to specify options). Good.

For "fetch", we end up in upload_pack_v2(). We write via process_haves_and_send_acks(), which will either:

  1. Write nothing and return 1.

  2. Write a delim and return 1.

  3. Write a flush packet and return 0.

In the final case, we jump to FETCH_DONE. So this really is the final thing we say. Good.

In the first two cases, we jump to FETCH_SEND_PACK. We may write out wanted-ref info, followed by a delim. OK. Then shallow info, followed by a delim. OK. Then the actual packfile via create_pack_file(). There our write behavior depends on use_sideband. If not set, we just dump data directly. If set, then we packetize. And it will always be set for v2 (we do it unconditionally in the beginning of upload_pack_v2()).

So we only need to care about use_sideband cases. And there we may send keepalives, but those are "0005\1" empty data packets. OK. We may send data via send_client_data(), but those will be real data packets. And then finally we give a single packet_flush(). Good.

So I think we can say that yes, the protocol as designed will send a flush at the end of every response, and will not ever send another flush.

_But_ that's if all goes well. If upload-pack sees an error, it may:

  - write an ERR packet in the earlier non-sideband steps, and then die

  - write a sideband 3 packet in later steps, and then die

  - just die() in some cases (e.g., pack-objects failed to start)

Obviously these are all an error on the client. And that third one especially is probably a hanging case that we'd like to fix with this series. But in the other two, we'd definitely want to make sure that remote-curl doesn't complain too early or too loudly, and that the error is shown to the user.

I think if remote-curl is just asking "did we see a flush packet at the end" then it is likely to complain unnecessarily in that case. And I don't think it should be inspecting packets for ERR or sideband-3. It doesn't know enough about the rest of the conversation to know which is correct. Though I guess we'd really only have to inspect the final packet.

I really wish we had converted all ERRs into packet 0002 as part of the v2 conversion. Then somebody reading only at the pktline level could truly understand or proxy a full conversation without understanding anything about what's in the pktlines. But it's too late for that now.

So I think our options are probably:

  1. detect flush packets in remote-curl, and either:

     a. don't print an error, just hang up. That prevents a hang in the
	caller and produces no extra message on a real error. It may be
	less informative than it could be if the connection hangs up
	(though we may print a curl error message, and the caller will
	at least say "the helper hung up")

     b. like (a), but always print an error; this is your original
	patch, but I _suspect_ (but didn't test) that it would produce
	extra useless messages for errors the server reports

     c. between the two: inspect the final packet data for evidence of
        ERR/sideband 3 and suppress any message if found

  2. helper signals end-of-response to caller (then it never produces a
     message itself; only the caller does, and it would abort on an ERR
     packet before then)

     a. using a special pktline (your "0002" patch)

     b. some other out-of-band mechanism (e.g., could be another fd)

I think this is pushing me towards 2a, your "0002" patch. It sidesteps the error-message questions entirely (and I think 2b is too convoluted to be worth pursuing, especially on Windows where setting up extra pipes is tricky). But I'd also be OK with 1a or 1c.

-Peff