Re: [PATCH 0/4] jk/version-string and google code

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2012, 19:11 UTC
Message-ID: <7vhasak1el.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAD0k6qSZYH5mvwR5PKhs1FFAPqMfRryXooxd_qhtt+eGXU7pUA@mail.gmail.com>

Dave Borowitz <dborowitz@google.com> writes:

Show 5 quoted lines

> 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.

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.