# [PATCH] transport-helper.c: don't leak fdopen'd stream buffers 4 messages from 2009-09-12 to 2009-09-13. Participants: Jim Meyering, Junio C Hamano, Johannes Sixt. Thread: https://gitlist.dev/t/20909 ## Jim Meyering, 2009-09-12 09:38 Subject: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers Message-ID: <87hbv833kd.fsf@meyering.net> URL: https://gitlist.dev/e/87hbv833kd.fsf%40meyering.net ``` 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 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 --- transport-helper.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) 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, 2009-09-13 02:27 Subject: Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers Message-ID: <7vtyz760lm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtyz760lm.fsf%40alter.siamese.dyndns.org In-Reply-To: <87hbv833kd.fsf@meyering.net> ``` Jim Meyering 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? ``` ## Jim Meyering, 2009-09-13 07:45 Subject: Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers Message-ID: <878wgjz3sp.fsf@meyering.net> URL: https://gitlist.dev/e/878wgjz3sp.fsf%40meyering.net In-Reply-To: <7vtyz760lm.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Jim Meyering 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"); > 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, 2009-09-13 15:20 Subject: Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers Message-ID: <200909131720.36853.j6t@kdbg.org> URL: https://gitlist.dev/e/200909131720.36853.j6t%40kdbg.org In-Reply-To: <7vtyz760lm.fsf@alter.siamese.dyndns.org> ``` On Sonntag, 13. September 2009, Junio C Hamano wrote: > Jim Meyering 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 ```