Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2009, 02:27 UTC
Message-ID: <7vtyz760lm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87hbv833kd.fsf@meyering.net>

Jim Meyering <jim@meyering.net> writes:

Show 19 quoted lines

> diff --git a/transport-helper.c b/transport-helper.c
> index f57e84c..0bbd014 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport)
>  		if (!strcmp(buf.buf, "fetch"))
>  			data->fetch = 1;
>  	}
> +	fclose (file);
>  	return data->helper;
>  }
>
> @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport,
>  		if (strbuf_getline(&buf, file, '\n') == EOF)
>  			exit(128); /* child died, message supplied already */
>  	}
> +	fclose (file);
>  	return 0;
>  }

The callchain of fetch_with_fetch() looks like:

    fetch_with_fetch()
        helper = get_helper();
        --> get_helper()
            - start helper with start_command();
            - read from helper->out until it sees an empty line;
            - break out of the loop;
        <-- return helper
        - file = xfdopen(helper->out) to get another FILE on the fd
        - read the rest of the output from helper->out via file

It seems to me that the fclose() in get_helper() will close the underlying fd and would break the caller, no?

I think "struct helper_data" should get a new FILE* field and once somebody creates a FILE* out of its helper->out, that FILE* can be passed around without a new xfdopen().

Or something like that.

Who is responsible for closing the underlying helper->out fd in the start_command() API, by the way?