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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 15, 2009, 20:45 UTC
Message-ID: <20091015204543.GP10505@spearce.org>
In-Reply-To: <7vfx9k4d33.fsf@alter.siamese.dyndns.org>

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

Show 8 quoted lines

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