{"thread":{"id":"20909","subject":"[PATCH] transport-helper.c: don't leak fdopen'd stream buffers","startedAt":"2009-09-12T09:38:58Z","lastAt":"2009-09-13T15:20:36Z","messageCount":4,"participants":["Jim Meyering","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"122939","messageId":"87hbv833kd.fsf@meyering.net","threadId":"20909","inReplyTo":null,"subject":"[PATCH] transport-helper.c: don't leak fdopen'd stream buffers","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-12T09:38:58Z","receivedAt":"2009-09-12T09:38:58Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Looking in the vicinity today, I noticed that these\nfdopen'd streams were never fclosed; technically a leak\nof both the FILE buffer and the file descriptor.\n\n>From aeae4edb1146b107f6a397118db8b0ac06b884d9 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Sat, 12 Sep 2009 11:35:17 +0200\nSubject: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers\n\n* transport-helper.c (get_helper, fetch_with_fetch, get_refs_list):\nCall fclose on each just-created FILE* pointer, when done.\n\nSigned-off-by: Jim Meyering \n---\n transport-helper.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f57e84c..0bbd014 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tif (!strcmp(buf.buf, \"fetch\"))\n \t\t\tdata->fetch = 1;\n \t}\n+\tfclose (file);\n \treturn data->helper;\n }\n\n@@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport,\n \t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n \t\t\texit(128); /* child died, message supplied already */\n \t}\n+\tfclose (file);\n \treturn 0;\n }\n\n@@ -147,6 +149,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \t\t\tget_sha1_hex(buf.buf, (*tail)->old_sha1);\n \t\ttail = &((*tail)->next);\n \t}\n+\tfclose (file);\n \tstrbuf_release(&buf);\n\n \tfor (posn = ret; posn; posn = posn->next)\n--\n1.6.5.rc0.190.g15871\n"},{"id":"123017","messageId":"7vtyz760lm.fsf@alter.siamese.dyndns.org","threadId":"20909","inReplyTo":"87hbv833kd.fsf@meyering.net","subject":"Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-13T02:27:01Z","receivedAt":"2009-09-13T02:27:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index f57e84c..0bbd014 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport)\n> \t\tif (!strcmp(buf.buf, \"fetch\"))\n> \t\t\tdata->fetch = 1;\n> \t}\n> +\tfclose (file);\n> \treturn data->helper;\n> }\n>\n> @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport,\n> \t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> \t\t\texit(128); /* child died, message supplied already */\n> \t}\n> +\tfclose (file);\n> \treturn 0;\n> }\n\nThe callchain of fetch_with_fetch() looks like:\n\n fetch_with_fetch()\n helper = get_helper();\n --> get_helper()\n - start helper with start_command();\n - read from helper->out until it sees an empty line;\n - break out of the loop;\n <-- return helper\n - file = xfdopen(helper->out) to get another FILE on the fd\n - read the rest of the output from helper->out via file\n\nIt seems to me that the fclose() in get_helper() will close the underlying\nfd and would break the caller, no?\n\nI think \"struct helper_data\" should get a new FILE* field and once\nsomebody creates a FILE* out of its helper->out, that FILE* can be passed\naround without a new xfdopen().\n\nOr something like that.\n\nWho is responsible for closing the underlying helper->out fd in the\nstart_command() API, by the way?\n"},{"id":"123039","messageId":"878wgjz3sp.fsf@meyering.net","threadId":"20909","inReplyTo":"7vtyz760lm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-13T07:45:10Z","receivedAt":"2009-09-13T07:45:10Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index f57e84c..0bbd014 100644\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport)\n>> \t\tif (!strcmp(buf.buf, \"fetch\"))\n>> \t\t\tdata->fetch = 1;\n>> \t}\n>> +\tfclose (file);\n>> \treturn data->helper;\n>> }\n>>\n>> @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport,\n>> \t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n>> \t\t\texit(128); /* child died, message supplied already */\n>> \t}\n>> +\tfclose (file);\n>> \treturn 0;\n>> }\n>\n> The callchain of fetch_with_fetch() looks like:\n>\n> fetch_with_fetch()\n> helper = get_helper();\n> --> get_helper()\n> - start helper with start_command();\n> - read from helper->out until it sees an empty line;\n> - break out of the loop;\n> <-- return helper\n> - file = xfdopen(helper->out) to get another FILE on the fd\n> - read the rest of the output from helper->out via file\n>\n> It seems to me that the fclose() in get_helper() will close the underlying\n> fd and would break the caller, no?\n\nI confess that my sole test was to run \"make test\", which passed.\nIf the fd must live on, a slightly less invasive change would be to\nxdup each descriptor just before each of the three xfdopen calls, e.g.,\n\n- file = xfdopen(helper->out, \"r\");\n+ file = xfdopen(xdup(helper->out), \"r\");\n\n> I think \"struct helper_data\" should get a new FILE* field and once\n> somebody creates a FILE* out of its helper->out, that FILE* can be passed\n> around without a new xfdopen().\n>\n> Or something like that.\n\nThat's probably best.\n\n> Who is responsible for closing the underlying helper->out fd in the\n> start_command() API, by the way?\n"},{"id":"123064","messageId":"200909131720.36853.j6t@kdbg.org","threadId":"20909","inReplyTo":"7vtyz760lm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] transport-helper.c: don't leak fdopen'd stream buffers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-09-13T15:20:36Z","receivedAt":"2009-09-13T15:20:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 13. September 2009, Junio C Hamano wrote:\n> Jim Meyering writes:\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index f57e84c..0bbd014 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct\n> > transport *transport) if (!strcmp(buf.buf, \"fetch\"))\n> > \t\t\tdata->fetch = 1;\n> > \t}\n> > +\tfclose (file);\n> > \treturn data->helper;\n> > }\n> >\n> > @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport\n> > *transport, if (strbuf_getline(&buf, file, '\\n') == EOF)\n> > \t\t\texit(128); /* child died, message supplied already */\n> > \t}\n> > +\tfclose (file);\n> > \treturn 0;\n> > }\n>\n> The callchain of fetch_with_fetch() looks like:\n>\n> fetch_with_fetch()\n> helper = get_helper();\n> --> get_helper()\n> - start helper with start_command();\n> - read from helper->out until it sees an empty line;\n> - break out of the loop;\n> <-- return helper\n> - file = xfdopen(helper->out) to get another FILE on the fd\n> - read the rest of the output from helper->out via file\n>\n> It seems to me that the fclose() in get_helper() will close the underlying\n> fd and would break the caller, no?\n>\n> I think \"struct helper_data\" should get a new FILE* field and once\n> somebody creates a FILE* out of its helper->out, that FILE* can be passed\n> around without a new xfdopen().\n>\n> Or something like that.\n>\n> Who is responsible for closing the underlying helper->out fd in the\n> start_command() API, by the way?\n\nA pipe was requested by setting helper->out = -1 before the call to \nstart_command(), and in such a case the caller must close the fd.\n\n-- Hannes\n"}]}