Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context

From: Jeff King <peff@peff.net>
Date: Dec 13, 2018, 08:04 UTC
Message-ID: <20181213080422.GA12132@sigill.intra.peff.net>
In-Reply-To: <CAJB1erXRqQW0yQyZutJAJKC7WbdVhBAYUMWM+8ZutxA-W-7S8w@mail.gmail.com>

On Wed, Dec 12, 2018 at 05:17:01PM -0800, Masaya Suzuki wrote:

Show 20 quoted lines

> > This is a change in the spec with an accompanying change in the code,
> > which raises the question: what do other implementations do with this
> > change (both older Git, and implementations like JGit, libgit2, etc)?
> 
> JGit is similar to Git. It parses "ERR " in limited places. When it sees an ERR
> packet in an unexpected place, it'll fail somewhere in the parsing code.
> 
> https://github.com/eclipse/jgit/blob/30c6c7542190c149e2aee792f992a312a5fc5793/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java#L145-L147
> https://github.com/eclipse/jgit/blob/f40b39345cd9b54473ee871bff401fe3d394ffe3/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java#L208
> 
> I'm not familiar with libgit2 code, but it seems it handles this at a
> lower level. An error type packet is parsed out at a low level, and
> the error handling is done by the callers of the packet parser.
> 
> https://github.com/libgit2/libgit2/blob/bea65980c7a42e34edfafbdc40b199ba7b2a564e/src/transports/smart_pkt.c#L482-L483
> 
> I cannot find an ERR packet handling in go-git. It seems to me that if
> an ERR packet appears it treats it as a parsing error.
> 
> https://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/common.go#L60-L62

Thanks for digging into these. It does make sense that other implementations would give a parsing error. Hopefully they also produce a sensible error message (ideally printing the bogus pktline), but even if they don't we're probably no worse off than the status quo. With the current scheme, the server can't give any message, and just has to hang up anyway.

Show 23 quoted lines

> > I think the answer for older Git is "hang up unceremoniously", which is
> > probably OK given the semantics of the change. And I'd suspect most
> > other implementations would do the same. I just wonder if anybody tested
> > it with other implementations.
> 
> I'm thinking aloud here. There would be two aspects of the protocol
> compatibility: (1) new clients speak to old servers (2) old clients
> speak to a new server that speaks the updated protocol.
> 
> For (1), I assume that in the Git pack protocol, a packet starting
> from "ERR " does not appear naturally except for a very special case
> that the server doesn't support sideband, but using the updated
> protocol. As you mentioned, at first it looks like this can mistakenly
> parse the pack file of git-receive-pack as an ERR packet, assuming
> that git-receive-pack's pack file is packetized. Actually
> git-receive-pack's pack file is not packetized in the Git pack
> protocol (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1695).
> I recently wrote a Git protocol parser
> (https://github.com/google/gitprotocolio), and I confirmed that this
> is the case at least for the HTTP transport. git-upload-pack's pack
> file is indeed packetized, but packetized with sideband. Except for
> the case where sideband is not used, the packfiles wouldn't be
> considered as an ERR packet accidentally.

Right, that matches my understanding.

Show 8 quoted lines

> For (2), if the old clients see an unexpected ERR packet, they cannot
> parse it. They would handle this unparsable data as if the server is
> not speaking Git protocol correctly. Even if the old clients just
> ignore the packet, due to the nature of the ERR packet, the server
> won't send further data. The client won't be able to proceed. Overall,
> the clients anyway face an error, and the only difference would be
> whether the clients can show an error nicely or not. The new clients
> will show the errors nicely to users. Old clients will not.

Yeah, this was the case I was more concerned about, and I think it is probably OK (by this rationale, and what I wrote above).

Show 6 quoted lines

> > So I think it's probably not a good idea to unconditionally have callers
> > of packet_read_with_status() handle this. We'd need a flag like
> > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.
> 
> This is outside of the Git pack protocol so having a separate parsing
> mode makes sense to me.

Yeah. Here's a sample script which works with current Git (the index contains the uppercased content "ERR FOO"), but fails after this patch (Git thinks the filter reported an error and dies; it's not great that we die in the packet-reading code at all for this case, but your patch is hardly the first call to die() in that function).

-- >8 -- git init -q repo cd repo

echo '*.magic filter=magic' >.git/info/attributes git config filter.magic.process $PWD/filter

# toy filter to uppercase content
cat >filter <<-\EOF
#!/usr/bin/perl
sub read_pkt {
  my @r;
  while (1) {
    read(STDIN, my $len, 4);
    last if $len eq "0000";
    read(STDIN, my $buf, hex($len)-4);
    push @r, $buf;
  }
  return @r;
}
sub write_pkt {
  local $| = 1;
  while (@_) {
    my $buf = shift;
    printf "%04x", length($buf) + 4;
    print $buf;
  }
  print "0000";
}

read_pkt(); # handshake write_pkt(qw(git-filter-server version=2)); read_pkt(); # capabilities write_pkt(qw(capability=clean));

read_pkt(); # clean command @content = read_pkt();

write_pkt(qw(status=success)); write_pkt(map { uc } @content); write_pkt(); # final status EOF chmod +x filter

echo 'err foo' >foo.magic git add foo.magic git cat-file blob :foo.magic