Re: [PATCH 1/1] upload-pack: fix race condition in error messages
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Aug 30, 2019, 12:10 UTC
- Message-ID
- <20190830121005.GI8571@szeder.dev>
- In-Reply-To
- <20190829220630.GH8571@szeder.dev>
On Fri, Aug 30, 2019 at 12:06:30AM +0200, SZEDER Gábor wrote:
Show 11 quoted lines
> On Thu, Aug 29, 2019 at 11:58:18PM +0200, SZEDER Gábor wrote: > > On Thu, Aug 29, 2019 at 10:38:05AM -0400, Jeff King wrote: > > > So any fixes there have to happen on the client side. I am still > > > confused about why the client is writing in this case, per the argument > > > in 014ade7484 (upload-pack: send ERR packet for non-tip objects, > > > 2019-04-13). It would be nice to use GIT_TRACE_PACKET to see what it's > > > trying to write, but I still haven't been able to reproduce the issue. > > > > It's the "done" line: > > > > + cat trace-packet
[...]
Show 5 quoted lines
> > packet: upload-pack> 0000 > > packet: fetch> done > > > > In the avarage successful run that "fetch> done" pkt-line is > > immediately after the "fetch> 0000".
So instead of immediately die()int after write_in_full() returned an error, fetch should first try to read all incoming packets in the hope that the remote did send an ERR packet before it died, and then die with the error in that packet, or fall back to the current generic error message if there is no ERR packet (e.g. remote segfaulted or something similarly horrible). This fixes the test failure with that strategically-placed sleep() in 'fetch-pack.c'.
https://travis-ci.org/szeder/git/jobs/578778749#L2689
Alas, passing a 'reader' to a function called send_request() doesn't look quite right, does it... And I'm not sure about the stateless communication, it still uses write_or_die().
diff --git a/fetch-pack.c b/fetch-pack.c index e18864458b..773d9c7904 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -186,14 +186,27 @@ static enum ack_type get_ack(struct packet_reader *reader, } static void send_request(struct fetch_pack_args *args, - int fd, struct strbuf *buf) + int fd, struct strbuf *buf, + struct packet_reader *reader) { if (args->stateless_rpc) { send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX); packet_flush(fd); } else { - if (write_in_full(fd, buf->buf, buf->len) < 0) + if (write_in_full(fd, buf->buf, buf->len) < 0) { + int save_errno = errno; + /* + * Read everything the remote has sent to us. + * If there is an ERR packet, then the loop die()s + * with the received error message. + * If we reach EOF without seeing an ERR, then die() + * with a generic error message, most likely "Broken + * pipe". + */ + while (packet_reader_read(reader) != PACKET_READ_EOF); + errno = save_errno; die_errno(_("unable to write to remote")); + } } } @@ -353,7 +366,7 @@ static int find_common(struct fetch_negotiator *negotiator, const char *arg; struct object_id oid; - send_request(args, fd[1], &req_buf); + send_request(args, fd[1], &req_buf, &reader); while (packet_reader_read(&reader) == PACKET_READ_NORMAL) { if (skip_prefix(reader.line, "shallow ", &arg)) { if (get_oid_hex(arg, &oid)) @@ -376,7 +389,7 @@ static int find_common(struct fetch_negotiator *negotiator, die(_("expected shallow/unshallow, got %s"), reader.line); } } else if (!args->stateless_rpc) - send_request(args, fd[1], &req_buf); + send_request(args, fd[1], &req_buf, &reader); if (!args->stateless_rpc) { /* If we aren't using the stateless-rpc interface @@ -398,7 +411,7 @@ static int find_common(struct fetch_negotiator *negotiator, int ack; packet_buf_flush(&req_buf); - send_request(args, fd[1], &req_buf); + send_request(args, fd[1], &req_buf, &reader); strbuf_setlen(&req_buf, state_len); flushes++; flush_at = next_flush(args->stateless_rpc, count); @@ -473,7 +486,7 @@ static int find_common(struct fetch_negotiator *negotiator, if (!got_ready || !no_done) { sleep(1); packet_buf_write(&req_buf, "done\n"); - send_request(args, fd[1], &req_buf); + send_request(args, fd[1], &req_buf, &reader); } print_verbose(args, _("done")); if (retval != 0) {