patch, 4 partsjk/version-string and google code

On Fri, Aug 03, 2012 at 12:19:16PM -0400, Jeff King wrote:

Ugh, the jk/version-string topic breaks fetching from Google Code. With my patch, the client unconditionally sends an "agent=foo" capability, but the server does not like seeing the unknown capability and ends the connection (I'm guessing with some kind of internal exception, since it spews "Internal server error" over the protocol channel).

This is the right thing to do according to protocol-capabilities.txt, which says:

  Client will then send a space separated list of capabilities it wants
  to be in effect. The client MUST NOT ask for capabilities the server
  did not say it supports.

  Server MUST diagnose and abort if capabilities it does not understand
  was sent.  Server MUST NOT ignore capabilities that client requested
  and server advertised.  As a consequence of these rules, server MUST
  NOT advertise capabilities it does not understand.

However, that is not how git-core behaves. Its server side will ignore an unknown capability coming from the client (so not only is it more lenient about what the client does, but it does not follow the "MUST" directives in the second paragraph).

This isn't a huge deal for this topic; any server that is collecting the data should be advertising anyway. The only ones who would miss out are humans trying to debug client behavior via tcpdump or similar, when the server side is an older version of git.

That's probably acceptable, given that the alternative is changing Google Code's implementation, along with finding out how many other implementations might have followed that spec strictly. We might or might not want to loosen the "MUST" bits in that document, since git itself does not follow them.

Here's a patch series that goes on top of jk/version-string:

  [1/4]: send-pack: fix capability-sending logic

This one is a minor bug fix in the same area.

  [2/4]: do not send client agent unless server does first

The actual fix.

  [3/4]: connect: learn to parse capabilities with values
  [4/4]: fetch-pack: mention server version with verbose output

A bonus feature. I'm not sure if they are worth doing or not. I'd really expect somebody debugging a protocol issue to just use GIT_TRACE_PACKET, and then they can read it straight from the packet dump themselves.

-Peff

If we have capabilities to send to the server, we send the regular "want" line followed by a NUL, then the capabilities; otherwise, we do not even send the NUL.

However, when checking whether we want to send the "quiet" capability, we check args->quiet, which is wrong. That flag only tells us whether the client side wanted to be quiet, not whether the server supports it (originally, in c207e34f, it meant both; however, that was later split into two flags by 01fdc21f).

We still check the right flag when actually printing "quiet", so this could only have two effects:

  1. We might send the trailing NUL when we do not otherwise
     need to. In theory, an antique pre-capability
     implementation of git might choke on this (since the
     client is instructed never to respond with capabilities
     that the server has not first advertised).

  2. We might also want to send the quiet flag if the
     args->progress flag is false, but this code path would
     not trigger in that instance.

In practice, it almost certainly never matters. The report-status capability dates back to 2005. Any real-world server is going to advertise that, and we will always respond with at least that capability.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm tempted to remove this part of the conditional entirely to make the
code simpler, which would mean that we always send the extra NUL, even
if there are no capabilities. But I'm not 100% sure that pre-1.1.0
versions of git actually handle that, and who knows if there are other
implementations. This fix is the safe, conservative route.

 builtin/send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Commit ff5effdf taught both clients and servers of the git protocol to send an "agent" capability that just advertises their version for statistics and debugging purposes. However, the protocol-capabilities.txt document indicates that the client's advertisement is actually a response, and should never include capabilities not mentioned in the server's advertisement.

Adding the unconditional advertisement in the server programs was OK, then, but the clients broke the protocol. The server implementation of git-core itself does not care, but at least one does: the Google Code git server will hang up with an internal error upon seeing an unknown capability.

Instead, each client must record whether we saw an agent string from the server, and respond with its agent only if the server mentioned it first.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c |  7 ++++++-
 builtin/send-pack.c  | 12 +++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

Just a couple of minor comments.

This is only set to false once per process invocation. We do not currently talk with more than one remote from the same process in the "fetch" codepath, and we must maintain that. This fix will be broken otherwise ("recursive submodule fetch" comes to mind; one more reason it should do its submodule business in a separate process).

This codepath still forgets to check if the other side advertised "thin-pack", "no-progress", and "include-tag", no?

This is initialied to 0 per communication, so having multiple remote.$there.pushURL configuration variables will work correctly.

On Fri, Aug 10, 2012 at 12:45:19PM -0700, Junio C Hamano wrote:

Right; I followed the existing options in both fetch-pack and send-pack (use_sideband, no_done, and others here have the same issue), with the assumption that the current code was not broken (which may not be true, if it is simply masked by the fact that in practice everybody happens to support those older features).

I don't know off-hand whether that is actually a trigger-able bug in the current code or not. It's probably a good topic for a follow-on series.

Yes. I didn't realize it until Dave mentioned the "innocuous" list. I think cleaning them up is reasonable, but probably a separate topic.

Right. Not through me having thought about it, though, but by following the existing convention.

-Peff

We already take care to parse a capability like "foo=bar" properly, but the code does not provide a good way of actually finding out what is on the right-hand side of the "=".

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  2 ++
 connect.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

On Fri, Aug 10, 2012 at 3:58 AM, Jeff King <peff@peff.net> wrote:

s/The cannot/The value cannot/

-- ES

Jeff King <peff@peff.net> writes:

Having to run strlen(feature) three times in this function (once in parse_feature_request() as part of strstr() and the edge check of the found string, once as part of prefixcmp() here, and then an explicit strlen() to skip it) makes me feel dirty.

It is not wrong per-se, but it is likely that the caller has a constant string as the feature when it called this function, so perhaps just changing the function signature of server_supports, i.e.

    const char *server_supports(const char *feature)
    {
	return parse_feature_request(server_capabilities, feature);
    }

to return "var=val " would be more than sufficient.

Then the existing callers can keep doing

	if (server_supports("thin-pack"))
        if (!server_supports("quiet"))

and a new caller can do something like

	agent = server_supports("agent");
        if (!agent || !agent[5])
        	... no agent ...
	else {
        	int span = strcspn(agent + 6, " \t\n");
                printf("I found agent=<%.*s>!\n", span, agent + 6);
	}

which doesn't look too bad.

On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:

I thought about that, but it seems like a quite premature optimization. It is three extra strlens per network conversation. _If_ you have turned on double-verbosity in fetch-pack. I considered reusing the existing parse_feature_request function more valuable from a maintenance standpoint.

I would think the extra memory allocation would dwarf it, anyway.

That was in fact my first iteration, but...

I didn't want to force callers to have to deal with ad-hoc parsing.

Anyway, do you think this is even worth doing at this point? I'm lukewarm on the final two patches due to the existence of GIT_TRACE_PACKET, which is much more likely to be useful.

-Peff

Jeff King <peff@peff.net> writes:

I forgot to mention it, but the above was done also to make it "possible but not mandatory" to pay extra allocation penalty. The caller can choose to parse the string into an int, for example, without extra allocation. Only the ones that want a string value and keep a copy around do have to do xmemdupz().

In the longer term, I think giving callers access to the parameter value given to a capability is necessary. If we had this facility in the old days, we wouldn't have done side-band-64k but spelled it as side-band=64k.

For the agent=<foo>, certainly we don't need it.

Junio C Hamano <gitster@pobox.com> writes:

Here are the first of two patches to replace your 3 and 4 without extra allocations, primarily for further discussion.

-- >8 --
Subject: [PATCH 3/4] connect: expose the parameter to a capability

We already take care to parse a capability like "foo=bar" properly, but the code does not provide a good way of actually finding out what is on the right-hand side of the "=".

Based on the patch by Jeff King <peff@peff.net>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h   |  1 +
 connect.c | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

Fetch-pack's verbose mode is more of a debugging mode (and in fact takes two "-v" arguments to trigger via the porcelain layer). Let's mention the server version as another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is your 4 adjusted for the previous one, releaving the
   caller from having to figure out where the capability string
   ends.

 builtin/fetch-pack.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Junio C Hamano <gitster@pobox.com> writes:

Oops; this was a cut and paste error. There are these four (counting the blank after "Subject:") lines before the description.

    From: Jeff King <peff@peff.net>
    Date: Fri, 10 Aug 2012 03:59:29 -0400
    Subject: [PATCH] fetch-pack: mention server version with verbose output

On Mon, Aug 13, 2012 at 12:07:35PM -0700, Junio C Hamano wrote:

Yeah, this is exactly the kind of ugliness I was trying to avoid with my allocating wrapper. Still, there is only one call site, so I do not care overly much (and I as I've already said, I'm lukewarm on the final two patches, anyway).

There is one difference in your code and mine. With mine, the server can say simply "agent" to tell the client that it understands the extension but does not wish to disclose its version. That might be considered unfriendly (why would the client show theirs if the server is not willing to do the same?), but it may be a practical decision (e.g., security policies may say that servers are higher-risk targets[1]). Of course, a server can also say "agent=git/none-of-your-business"; this is just a syntactic question.

-Peff

[1] I think you and I both agreed earlier that the "sharing versions is
    a security risk" line of argument is not that compelling, but that
    does not mean it is not made all the time.

-Peff

Jeff King <peff@peff.net> writes:

You do not even have to advertise it in the first place, no?

On Mon, Aug 13, 2012 at 02:07:22PM -0700, Junio C Hamano wrote:

If you want the client to respond with theirs, you do.

-Peff

Jeff King <peff@peff.net> writes:

Actually, the above is vastly superiour compared to the allocating kind. Be honest and think about it. If the caller wants to allocate, it could, and it does not even have to count. If the caller does not want to allocate, it does not have to pay the price.

On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:

My point is that the run-time allocation price is quite small, but the readability cost of that ugly conditional with the magic "5" is non-trivial. But they are apples and oranges, so it is hard to compare their amounts directly.

-Peff

On Mon, Aug 13, 2012 at 05:11:10PM -0400, Jeff King wrote:

So if we want to avoid the allocation, then this is how I would do it: by returning the feature's _value_ and not the whole key. Since we know that the beginning part must obviously match what we fed it anyway, it is not that interesting.

-- >8 --
Subject: [PATCH] parse_feature_request: make it easier to see feature values

We already take care to parse key/value capabilities like "foo=bar", but the code does not provide a good way of actually finding out what is on the right-hand side of the "=".

A server using "parse_feature_request" could accomplish this with some extra parsing. You must skip past the "key" portion manually, check for "=" versus NUL or space, and then find the length by searching for the next space (or NUL). But clients can't even do that, since the "server_supports" interface does not even return the pointer.

Instead, let's have our parser share more information by providing a pointer to the value and its length. The "parse_feature_value" function returns a pointer to the feature's value portion, along with the length of the value. If the feature is missing, NULL is returned. If it does not have an "=", then a zero-length value is returned.

Similarly, "server_feature_value" behaves in the same way, but always checks the static server_feature_list variable.

We can then implement "server_supports" in terms of "server_feature_value". We cannot implement the original "parse_feature_request" in terms of our new function, because it returned a pointer to the beginning of the feature. However, no callers actually cared about the value of the returned pointer, so we can simplify it to a boolean just as we do for "server_supports".

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  4 +++-
 connect.c | 45 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 10 deletions(-)

On Mon, Aug 13, 2012 at 09:59:27PM -0400, Jeff King wrote:

And here is the rebased 4/4 on top of that.

At this point, I think this part of the topic has received more than enough attention. Please feel free to apply these patches, your patches, or even just drop it altogether (and when somebody has a more compelling reason to actually parse such a value, they can resurrect the infrastructure patch).

-- >8 --
Subject: [PATCH] fetch-pack: mention server version with verbose output

Fetch-pack's verbose mode is more of a debugging mode (and in fact takes two "-v" arguments to trigger via the porcelain layer). Let's mention the server version as another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Jeff King <peff@peff.net> writes:

OK. The one I queued in 'pu' said "Server version not disclosed" when length was 0, but I think I like this one better.

Also I like the the update to the parsing code in the previous patch. It makes the logic clearer.

Thanks.

Fetch-pack's verbose mode is more of a debugging mode (and in fact takes two "-v" arguments to trigger via the porcelain layer). Let's mention the server version as another possible item of interest.

Signed-off-by: Jeff King <peff@peff.net>
---
Like I mentioned before, I'm lukewarm on this one, because I doubt
anybody actually uses this debugging information. If we drop it, it
makes sense to drop 3/4, too, which is just infrastructure for this.

 builtin/fetch-pack.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Jeff King <peff@peff.net> writes:

The fix looks sane and the right thing to do.

I've been using 'next' and started seeing this breakage when pushing to code.google.com only a few days ago. My reflog tells that ff5effd (include agent identifier in capability string, 2012-08-03) was merged to my everyday-work branch on this Monday, which is more or less consistent with what I am observing.

Yeah, we probably should fix in the implementation to honor the "MUST". When various implementations of clients can start asking something the server does not support and still yet expect the request to cause the server do unusual things, it will lead to chaos, and honoring that "MUST" is a good way to catch and help diagnosing such a breakage early.

But that is a separate topic and a longer term direction item.

Thanks.

On Fri, Aug 10, 2012 at 08:34:45AM -0700, Junio C Hamano wrote:

Thanks for confirming the push side. I have been running with the patch for months, but only recently happened to try cloning something from code.google.com. I assumed the push side had the same problem, but did not want to make a new project just to test (and it seemed fairly obvious that it would have the same issue).

It was pure coincidence that it happened at the same time you were graduating the topic to next. But I'm glad I was able to catch it before anybody started complaining. :)

-Peff

Jeff King <peff@peff.net> writes:

Note that I didn't "confirm" the fix. I only confirmed the existence of the breakage (not that I have any reason to doubt the competence your "fix" patches were made with).

When evaluating a change in the interoperability area, it does not add much more confidence to the correctness that the change has been in use for months with the same partner than that it has been used to talk to many different partners even for a short period of time, I guess.

Yeah, the problematic one is not in 'master'.

On Fri, Aug 10, 2012 at 11:52:28AM -0700, Junio C Hamano wrote:

Traditionally our interoperability testing has been to cook things in "next" and "master" and see if anybody complains. It would be nice to have an interoperability test suite that could hit some common hosting sites, as well as older versions of git-core itself. I suspect automating that would be a big pain, though.

-Peff

On Fri, Aug 10, 2012 at 2:50 PM, Jeff King <peff@peff.net> wrote:

I don't know that you need to hit the hosting sites themselves, just the implementations they are using. Dulwich and JGit are both open source. It should be possible to build a smallish compatibility test suite that grabs everyone into one tree, compiles a small test server from each, and runs the matrix locally. Then its up to the hosting sites to worry about making sure their implementations are included in this suite, and up-to-date. :-)

Shawn Pearce <spearce@spearce.org> writes:

Yeah, and do that for all the major versions of these implementations.

Jeff King <peff@peff.net> writes:

I asked the folks who run code.google.com and they are indeed seeing something like these in their logs:

 >> Client asked for capability agent=git/1.7.12.rc2.79.g86c1702 that was not advertised.

So please consider your conjecture confirmed, and thanks for a prompt fix.

On Fri, Aug 10, 2012 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

FWIW, this error comes from Dulwich: https://github.com/jelmer/dulwich/blob/25250c1694dac343d469742aeafa139f37fc4ec6/dulwich/server.py#L196

So any servers running Dulwich would be affected by this...though I'm not aware of any large-scale Dulwich installations other than Google Code.

On Fri, Aug 10, 2012 at 11:06:08AM -0700, Dave Borowitz wrote:

Thanks for the data point. I knew you guys ran some custom code, so I wasn't sure how widespread this is. The fact that other dulwich-based servers would see the same issue makes me doubly sure that my fix is the right direction.

I'd rather not break small-scale installations, either. :)

-Peff

On Fri, Aug 10, 2012 at 11:08 AM, Jeff King <peff@peff.net> wrote:

You may also notice in that code a set of innocuous_capabilities, which IIRC is the complete set of capabilities, at the time of writing, that the C git client may send without the server advertising them. Such a set (painstakingly assembled, I assure you :) may be useful as we move further in this direction.

On Fri, Aug 10, 2012 at 11:13:30AM -0700, Dave Borowitz wrote:

Oh, hmm. When initially writing my message I thought that might be the case, but I checked to see that the features were sent only when the server had first advertised them. However, I didn't notice that is true only in _some_ of these lines from fetch-pack.c:

    if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
    if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
    if (no_done)            strbuf_addstr(&c, " no-done");
    if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
    if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
    if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
    if (args.no_progress)   strbuf_addstr(&c, " no-progress");
    if (args.include_tag)   strbuf_addstr(&c, " include-tag");

The early ones are checking that the server claimed support, but all of the args.* ones are influenced directly by the arguments, whether the server supports it or not.

I don't think there's any bug here. They are all of a class of features where the client can handle the case where the server simply ignores the request. However it is certainly food for thought if we are considering tightening git's server side (even if we fix these, we have to support the innocuous capabilities list forever for older clients).

-Peff

Jeff King <peff@peff.net> writes:

I doubt the "innocuous" approach is really viable, unless we have an autoritative documentation that tells which ones are and which ones are not innocuous, and everybody follows it, so that everybody's server and client understands the same set of capabilities as such.

Which is not likely to happen. So in that sense, the above have three bugs. A new person that starts writing his server without knowing the workaround Dulwich used that has been hidden from the Git community until today will have to rediscover the "innocuous" workaround on his server, unless such buggy clients die out.

I'd rather make sure that 10 years on, the maintainer does not have to worry about interoperating with a new server written by some third-party.

Something like this, perhaps.

 builtin/fetch-pack.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Fri, Aug 10, 2012 at 02:25:51PM -0700, Junio C Hamano wrote:

I think the point is that the ordering is something like:

  1. New client features get implemented wrongly. Nobody notices because
     the server side is lax.

  2. Somebody writes a new server (dulwich), or tightens the existing
     code (what we are thinking of). They create the innocuous list
     because they must deal with older clients from (1).

  3. Somebody tries to implement a new client feature wrongly. They
     notice because strict servers actually exist, and are told their
     client is buggy and wrong. The innocuous list never grows.

So we are at step (2), and are just realizing the client problem. Even if we fix it, we still need the current innocuous list to handle existing clients.

Although I would think you do not have to worry about the innocuous list if you always advertise those features. Which I'm surprised dulwich does not do (IOW, why do they even need the innocuous list?).

Oh, definitely. I wasn't arguing that we shouldn't fix the clients. Just that we need to make sure that the current list continues working if we decide to tighten the server side.

Yes, I think that is all that is necessary to fix the immediate issue. The protocol-capabilities document talks about what to do when include-tag is not available ("SHOULD issue a subsequent fetch to acquire the tags that include-tag would have otherwise given the client"), but I am not sure how well we handle that (in theory we should be handling it already, but I didn't look).

-Peff

Jeff King <peff@peff.net> writes:

Yeah, it is more like "A separate fetch only to follow tags was the only way we handled it before we added include-tag, so we know it used to work, and also we know the four combinations of new/old * fetch/upload all used to work".

We may have broken some combinations over time, though.

Dave Borowitz <dborowitz@google.com> writes:

In builtin/fetch-pack.c, we find this:

		if (!fetching) {
			struct strbuf c = STRBUF_INIT;
			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
			if (no_done)            strbuf_addstr(&c, " no-done");
			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
			if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
			strbuf_release(&c);
		} else
			packet_buf_write(&req_buf, "want %s\n", remote_hex);

The ones we choose to throw at the other end based on "args.foo" are not protected by "server_supports()" at all, which is where the hardcoded list of "innocuous capabilities" comes from. I would say this is a client bug. I wish Dulwich folks didn't choose to be silent when they added that hardcoded list as a workaround.

If a client threw a request X at a server that does not support it, and relied on a server bug that does not reject such a request to allow it send a pack stream that does not conform to what X asked, and handled the pack stream assuming that the server did X, it would be a triple bug on the client's end. Depending on the nature of X, the end result may be broken. Dulwich is correct to raise an exception upon seeing agent=foo.

One could argue that from correctness standpoint, being asked to send ofs-delta and using only ref-delta does not make a corrupt packfile (it just makes things less efficient), but we cannot guarantee that all protocol capabilities will be "innocuous" that way. Longer term direction should be to reduce the "innocuous" set.

Thanks.