{"thread":{"id":"42288","subject":"[PATCH 1/2] am: plug small memory leak when split_mail_stgit_series() fails","startedAt":"2016-05-11T23:35:45Z","lastAt":"2016-05-12T15:59:17Z","messageCount":8,"participants":["Junio C Hamano","Jeff King","Mikael Magnusson","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"286298","messageId":"20160511233546.13090-1-gitster@pobox.com","threadId":"42288","inReplyTo":null,"subject":"[PATCH 1/2] am: plug small memory leak when split_mail_stgit_series() fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-11T23:35:45Z","receivedAt":"2016-05-11T23:35:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n builtin/am.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex ec75906..f1a84c6 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -842,9 +842,11 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,\n \tseries_dir = dirname(series_dir_buf);\n \n \tfp = fopen(*paths, \"r\");\n-\tif (!fp)\n+\tif (!fp) {\n+\t\tfree(series_dir_buf);\n \t\treturn error(_(\"could not open '%s' for reading: %s\"), *paths,\n \t\t\t\tstrerror(errno));\n+\t}\n \n \twhile (!strbuf_getline(&sb, fp, '\\n')) {\n \t\tif (*sb.buf == '#')\n-- \n2.8.2-679-g91c6421\n"},{"id":"286299","messageId":"20160511233546.13090-2-gitster@pobox.com","threadId":"42288","inReplyTo":"20160511233546.13090-1-gitster@pobox.com","subject":"[PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-11T23:35:46Z","receivedAt":"2016-05-11T23:35:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n builtin/am.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f1a84c6..a373928 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -761,9 +761,11 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n \t\tmail = mkpath(\"%s/%0*d\", state->dir, state->prec, i + 1);\n \n \t\tout = fopen(mail, \"w\");\n-\t\tif (!out)\n+\t\tif (!out) {\n+\t\t\tfclose(in);\n \t\t\treturn error(_(\"could not open '%s' for writing: %s\"),\n \t\t\t\t\tmail, strerror(errno));\n+\t\t}\n \n \t\tret = fn(out, in, keep_cr);\n \n-- \n2.8.2-679-g91c6421\n"},{"id":"286306","messageId":"20160512044730.GA5436@sigill.intra.peff.net","threadId":"42288","inReplyTo":"20160511233546.13090-2-gitster@pobox.com","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T04:47:30Z","receivedAt":"2016-05-12T04:47:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 11, 2016 at 04:35:46PM -0700, Junio C Hamano wrote:\n\n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/am.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index f1a84c6..a373928 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -761,9 +761,11 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n> \t\tmail = mkpath(\"%s/%0*d\", state->dir, state->prec, i + 1);\n> \n> \t\tout = fopen(mail, \"w\");\n> -\t\tif (!out)\n> +\t\tif (!out) {\n> +\t\t\tfclose(in);\n> \t\t\treturn error(_(\"could not open '%s' for writing: %s\"),\n> \t\t\t\t\tmail, strerror(errno));\n> +\t\t}\n\nPresumably `fclose` doesn't ever overwrite errno in practice, but I\nguess it could in theory.\n\nI also found it weird that we might fclose(stdin) via this line, but\nthat matches what happens in the non-error path, so I guess it's OK?\n\n-Peff\n"},{"id":"286307","messageId":"CAHYJk3Q90MrV_hxF+xxbFnJtL6_OLqTRoekwjc9-_LJuFc-aTg@mail.gmail.com","threadId":"42288","inReplyTo":"20160512044730.GA5436@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Mikael Magnusson","fromEmail":"mikachu@gmail.com","sentAt":"2016-05-12T05:23:02Z","receivedAt":"2016-05-12T05:23:02Z","isPatch":true,"sender":{"key":"mikachu@gmail.com","avatar":null},"body":"On Thu, May 12, 2016 at 6:47 AM, Jeff King wrote:\n> On Wed, May 11, 2016 at 04:35:46PM -0700, Junio C Hamano wrote:\n>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> builtin/am.c | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index f1a84c6..a373928 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -761,9 +761,11 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n>> mail = mkpath(\"%s/%0*d\", state->dir, state->prec, i + 1);\n>>\n>> out = fopen(mail, \"w\");\n>> - if (!out)\n>> + if (!out) {\n>> + fclose(in);\n>> return error(_(\"could not open '%s' for writing: %s\"),\n>> mail, strerror(errno));\n>> + }\n>\n> Presumably `fclose` doesn't ever overwrite errno in practice, but I\n> guess it could in theory.\n\nIt probably does pretty often in general, but not when the file is\nopened for input only.\n\n-- \nMikael Magnusson\n"},{"id":"286308","messageId":"20160512052909.GA18330@sigill.intra.peff.net","threadId":"42288","inReplyTo":"CAHYJk3Q90MrV_hxF+xxbFnJtL6_OLqTRoekwjc9-_LJuFc-aTg@mail.gmail.com","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T05:29:10Z","receivedAt":"2016-05-12T05:29:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2016 at 07:23:02AM +0200, Mikael Magnusson wrote:\n\n> >> - if (!out)\n> >> + if (!out) {\n> >> + fclose(in);\n> >> return error(_(\"could not open '%s' for writing: %s\"),\n> >> mail, strerror(errno));\n> >> + }\n> >\n> > Presumably `fclose` doesn't ever overwrite errno in practice, but I\n> > guess it could in theory.\n> \n> It probably does pretty often in general, but not when the file is\n> opened for input only.\n\nRight, I should have said \"this fclose\".\n\nI think EBADF is the only likely error when closing input, and that's\npresumably impossible here.\n\n-Peff\n"},{"id":"286321","messageId":"20160512075939.GA31343@dcvr.yhbt.net","threadId":"42288","inReplyTo":"20160512044730.GA5436@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-12T07:59:39Z","receivedAt":"2016-05-12T07:59:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Wed, May 11, 2016 at 04:35:46PM -0700, Junio C Hamano wrote:\n> > +++ b/builtin/am.c\n> > @@ -761,9 +761,11 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n> > \t\tmail = mkpath(\"%s/%0*d\", state->dir, state->prec, i + 1);\n> > \n> > \t\tout = fopen(mail, \"w\");\n> > -\t\tif (!out)\n> > +\t\tif (!out) {\n> > +\t\t\tfclose(in);\n> > \t\t\treturn error(_(\"could not open '%s' for writing: %s\"),\n> > \t\t\t\t\tmail, strerror(errno));\n> > +\t\t}\n> \n> Presumably `fclose` doesn't ever overwrite errno in practice, but I\n> guess it could in theory.\n\nI think both patches in this series would benefit from capturing\nerrno before cleanup. `fclose` can call `free`, and `free` could\ndo any manner of things such as calling `madvise` with a flag\nnot implemented in the running kernel, or failing an optional\ntrylock without being fatal.\n\nThere's lots of non-standard malloc implementations out there :)\n\nSo I'm not sure if there's ever a guarantee that a non-error\nfunction call preserves `errno`.\n"},{"id":"286322","messageId":"20160512080331.GA18874@sigill.intra.peff.net","threadId":"42288","inReplyTo":"20160512075939.GA31343@dcvr.yhbt.net","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T08:03:31Z","receivedAt":"2016-05-12T08:03:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2016 at 07:59:39AM +0000, Eric Wong wrote:\n\n> I think both patches in this series would benefit from capturing\n> errno before cleanup. `fclose` can call `free`, and `free` could\n> do any manner of things such as calling `madvise` with a flag\n> not implemented in the running kernel, or failing an optional\n> trylock without being fatal.\n> \n> There's lots of non-standard malloc implementations out there :)\n> \n> So I'm not sure if there's ever a guarantee that a non-error\n> function call preserves `errno`.\n\nGood point. This came up not too long ago in:\n\n http://article.gmane.org/gmane.comp.version-control.git/286460\n\nI believe POSIX does say that non-error calls should preserve errno, but\nall the world is not POSIX. And a future POSIX will mandate that `free`\nshould not touch errno, but it's not the future yet (and also, all the\nworld's not POSIX).\n\n-Peff\n"},{"id":"286335","messageId":"xmqqk2izgtd6.fsf@gitster.mtv.corp.google.com","threadId":"42288","inReplyTo":"20160512044730.GA5436@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] am: plug FILE * leak in split_mail_conv()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-12T15:59:17Z","receivedAt":"2016-05-12T15:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Presumably `fclose` doesn't ever overwrite errno in practice, but I\n> guess it could in theory.\n\nYeah, these two patches share the same issue.\n"}]}