Re: [RFC PATCH v3 00/17] Return of smart HTTP

["Shawn O. Pearce" <spearce@spearce.org>]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > It does.  It is caused by the disconnect_helper call inside of
> > fetch_with_import.  You can't disconnect inside of the fetch method
> > of a transport, the caller is going to disconnect you a second time.
> > ...
> > This bug isn't due to the merge, its a bug in Johan's series that
> > needs to be fixed before it could merge down to next/master.
...
> I am a bit confused about your diagnosis, though.  As far as I recall,
> Johan's topic itself nor 'pu' with Johan's topic but without v2 of
> sp/smart-http did not have the issue.

Sadly, sometimes double frees do not result in segfaults, other
times they do.  The reason you are not seeing a problem with these
other variants is because of luck, not code correctness.

Actually, after some further research, the bug is not Johan's but is
actually Daniel's.  Johan, I apologize for claiming it was your bug.

In:

  commit 23a3380ee9c2d5164712c40f8821cb0fba24e80c
  Author: Daniel Barkalow <barkalow@iabervon.org>
  Date:   Thu Sep 3 22:14:01 2009 -0400

    Add support for "import" helper command

Daniel introduces the fetch_with_import() function to
transport-helper.c.  This method calls disconnect_helper():

+static int fetch_with_import(struct transport *transport,
+                            int nr_heads, struct ref **to_fetch)
+{
...
+       disconnect_helper(transport);
+       finish_command(&fastimport);

Unfortunately this is in the middle of the transport_fetch() call
stack; transport_fetch() called the static fetch() function in
transport-helper.c, which in turn called fetch_with_import().

Callers (e.g. builtin-fetch.c) invoke transport_close() when
they are done with the handle (see line 704).  That in turn calls
disconnect_helper() a second time.

The disconnect_helper function is not prepared to be called twice:

static int disconnect_helper(struct transport *transport)
{
	struct helper_data *data = transport->data;
	if (data->helper) {
	...
	}
	free(data);
	return 0;
}

Because of that unexpected invocation inside of fetch_with_import
we have already free'd the memory block used by transport->data,
and the second invocation attempts to free it again.  Worse, if the
block was reused by a subsequent malloc, data->helper might not be
NULL, and we'd enter into the if block and do its work again.

Long story short, transport_close() is what is supposed to perform
the work that disconnect_helper does, as its the final thing right
before we free the struct transport block.  Free'ing the data block
inside of the fetch or push functions is wrong.

Its fine to close the helper and restart it within the single
lifespan of a struct transport, but dammit, don't free the
struct helper_data until transport_close().

-- 
Shawn.