threads / patch / 20909

patchtransport-helper.c: don't leak fdopen'd stream buffers

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

## tl;dr

4 messages between Sep 12, 2009 and Sep 13, 2009. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jim Meyering· Sep 12, 2009, 09:38 UTC · lore

Looking in the vicinity today, I noticed that these fdopen'd streams were never fclosed; technically a leak of both the FILE buffer and the file descriptor.

>From aeae4edb1146b107f6a397118db8b0ac06b884d9 Mon Sep 17 00:00:00 2001

From: Jim Meyering <meyering@redhat.com>
Date: Sat, 12 Sep 2009 11:35:17 +0200
Subject: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers

* transport-helper.c (get_helper, fetch_with_fetch, get_refs_list):
Call fclose on each just-created FILE* pointer, when done.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 transport-helper.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Show changes to transport-helper.c +3 −1

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;
 }

@@ -147,6 +149,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 			get_sha1_hex(buf.buf, (*tail)->old_sha1);
 		tail = &((*tail)->next);
 	}
+	fclose (file);
 	strbuf_release(&buf);

 	for (posn = ret; posn; posn = posn->next)
--
1.6.5.rc0.190.g15871

Junio C Hamano· Sep 13, 2009, 02:27 UTC · re: Jim Meyering · lore

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

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?

Jim Meyering· Sep 13, 2009, 07:45 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

Show 36 quoted lines

> Jim Meyering <jim@meyering.net> writes:
>
>> 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 confess that my sole test was to run "make test", which passed. If the fd must live on, a slightly less invasive change would be to xdup each descriptor just before each of the three xfdopen calls, e.g.,

-       file = xfdopen(helper->out, "r");
+       file = xfdopen(xdup(helper->out), "r");

Show 5 quoted lines

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

That's probably best.

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

Johannes Sixt· Sep 13, 2009, 15:20 UTC · re: Junio C Hamano · lore

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

On Sonntag, 13. September 2009, Junio C Hamano wrote:

Show 44 quoted lines

> Jim Meyering <jim@meyering.net> writes:
> > 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?

A pipe was requested by setting helper->out = -1 before the call to start_command(), and in such a case the caller must close the fd.

-- Hannes