{"thread":{"id":"65875","subject":"[PATCH] history: streamline message preparation and plug file stream leak","startedAt":"2026-06-26T16:38:44Z","lastAt":"2026-06-30T12:25:07Z","messageCount":6,"participants":["Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"546487","messageId":"xmqqecht8df1.fsf@gitster.g","threadId":"65875","inReplyTo":null,"subject":"[PATCH] history: streamline message preparation and plug file stream leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-26T16:38:42Z","receivedAt":"2026-06-26T16:38:44Z","isPatch":true,"body":"An early part of fill_commit_mmessage() function uses write_file_buf()\nto write out what was prepared in a strbuf, which is primarily meant\nfor use by callers that have their own message prepared fully and\ncalled as the last thing to flush it to the destination file.\n\nHowever, the function then opens a file stream in append mode to\nfurther write into it. It may have been understandable if this was\na later addition, but it seems it came from a single commit,\nd205234c (builtin/history: implement \"reword\" subcommand,\n2026-01-13), which is somewhat puzzling, but anyway...\n\nJust open the file stream upfront for writing, write the message\nthe function has in the strbuf, and then keep writing whatever it\nwants to write to the same open file stream.\n\nAnd do not forget to close the stream. We are about to pass the\nresulting file to an external editor, and on some systems, notably\nWindows, you are not supposed to keep a file open while expecting\nanother program to access it.\n\nDiagnosed-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n * As the initial one was written and sent as \"how about doing a bit\n more thorough job while we are at it?\" response to a posted patch\n found in ,\n this is a tested and merge-ready cersion that I consider \"v1\".\n\n builtin/history.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 8dcb9a6046..f17ec049c0 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,\n \t\t \" empty message aborts the commit.\\n\");\n \tstruct wt_status s;\n \n-\tstrbuf_addstr(out, default_message);\n-\tstrbuf_addch(out, '\\n');\n-\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n-\twrite_file_buf(path, out->buf, out->len);\n-\n \twt_status_prepare(repo, &s);\n \tFREE_AND_NULL(s.branch);\n \ts.ahead_behind_flags = AHEAD_BEHIND_QUICK;\n@@ -57,14 +52,20 @@ static int fill_commit_message(struct repository *repo,\n \ts.whence = FROM_COMMIT;\n \ts.committable = 1;\n \n-\ts.fp = fopen(git_path_commit_editmsg(), \"a\");\n+\ts.fp = fopen(path, \"w\");\n \tif (!s.fp)\n-\t\treturn error_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n+\t\treturn error_errno(_(\"could not open '%s'\"), path);\n+\n+\tstrbuf_addstr(out, default_message);\n+\tstrbuf_addch(out, '\\n');\n+\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n+\tfwrite(out->buf, 1, out->len, s.fp);\n \n \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n \twt_status_print(&s);\n \twt_status_collect_free_buffers(&s);\n \tstring_list_clear_func(&s.change, change_data_free);\n+\tfclose(s.fp);\n \n \tstrbuf_reset(out);\n \tif (launch_editor(path, out, NULL)) {\n-- \n2.55.0-rc2-177-gc9430f6415\n\n"},{"id":"546629","messageId":"akIRrmD4Tqp-Gi9d@pks.im","threadId":"65875","inReplyTo":"xmqqecht8df1.fsf@gitster.g","subject":"Re: [PATCH] history: streamline message preparation and plug file stream leak","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-29T06:33:18Z","receivedAt":"2026-06-29T06:33:25Z","isPatch":true,"body":"On Fri, Jun 26, 2026 at 09:38:42AM -0700, Junio C Hamano wrote:\n> diff --git a/builtin/history.c b/builtin/history.c\n> index 8dcb9a6046..f17ec049c0 100644\n> --- a/builtin/history.c\n> +++ b/builtin/history.c\n> @@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,\n> \t\t \" empty message aborts the commit.\\n\");\n> \tstruct wt_status s;\n> \n> -\tstrbuf_addstr(out, default_message);\n> -\tstrbuf_addch(out, '\\n');\n> -\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n> -\twrite_file_buf(path, out->buf, out->len);\n> -\n> \twt_status_prepare(repo, &s);\n> \tFREE_AND_NULL(s.branch);\n> \ts.ahead_behind_flags = AHEAD_BEHIND_QUICK;\n> @@ -57,14 +52,20 @@ static int fill_commit_message(struct repository *repo,\n> \ts.whence = FROM_COMMIT;\n> \ts.committable = 1;\n> \n> -\ts.fp = fopen(git_path_commit_editmsg(), \"a\");\n\nHere we reuse the local `path` variable, which already carries the\nresult of `git_path_commit_editmsg()`.\n\n> +\ts.fp = fopen(path, \"w\");\n> \tif (!s.fp)\n> -\t\treturn error_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n> +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n\nLikewise.\n\n> +\tstrbuf_addstr(out, default_message);\n> +\tstrbuf_addch(out, '\\n');\n> +\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n> +\tfwrite(out->buf, 1, out->len, s.fp);\n> \n> \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n> \twt_status_print(&s);\n> \twt_status_collect_free_buffers(&s);\n> \tstring_list_clear_func(&s.change, change_data_free);\n> +\tfclose(s.fp);\n\nThis is fixing the leaked file descriptor.\n\nOne thing I wonder though is that we don't perform any error checking on\nthe file in the new version. Previously, we would have died in case\n`write_file_buf()` failed. But now we just `fwrite()` without error\nchecking. I don't think that \"wt-status.c\" does error checking either,\nso we might end up with a partially-written file without us noticing.\n\nThanks!\n\nPatrick\n"},{"id":"546674","messageId":"xmqq33y5z82l.fsf@gitster.g","threadId":"65875","inReplyTo":"akIRrmD4Tqp-Gi9d@pks.im","subject":"Re: [PATCH] history: streamline message preparation and plug file stream leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-29T15:21:06Z","receivedAt":"2026-06-29T15:21:08Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n>> +\tstrbuf_addstr(out, default_message);\n>> +\tstrbuf_addch(out, '\\n');\n>> +\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n>> +\tfwrite(out->buf, 1, out->len, s.fp);\n>> \n>> \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n>> \twt_status_print(&s);\n>> \twt_status_collect_free_buffers(&s);\n>> \tstring_list_clear_func(&s.change, change_data_free);\n>> +\tfclose(s.fp);\n>\n> This is fixing the leaked file descriptor.\n>\n> One thing I wonder though is that we don't perform any error checking on\n> the file in the new version. Previously, we would have died in case\n> `write_file_buf()` failed. But now we just `fwrite()` without error\n> checking. I don't think that \"wt-status.c\" does error checking either,\n> so we might end up with a partially-written file without us noticing.\n\nYes, the fwrite() should be protected with an error checking and\ndie() the same way as the code before. Will send a v2.\n\nBut isn't the end result the same between preimage and postimage?\nIf the stuff appended by wt_status_* are still written without error\nchecking, we would leave a partially-written file that has the\ndefault_messages and the commented hint/action but not necessarily\nwhatever we wanted to add with wt_status().\n"},{"id":"546677","messageId":"akKXfQFw3RwkshG9@pks.im","threadId":"65875","inReplyTo":"xmqq33y5z82l.fsf@gitster.g","subject":"Re: [PATCH] history: streamline message preparation and plug file stream leak","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-29T16:04:13Z","receivedAt":"2026-06-29T16:04:19Z","isPatch":true,"body":"On Mon, Jun 29, 2026 at 08:21:06AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> +\tstrbuf_addstr(out, default_message);\n> >> +\tstrbuf_addch(out, '\\n');\n> >> +\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n> >> +\tfwrite(out->buf, 1, out->len, s.fp);\n> >> \n> >> \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n> >> \twt_status_print(&s);\n> >> \twt_status_collect_free_buffers(&s);\n> >> \tstring_list_clear_func(&s.change, change_data_free);\n> >> +\tfclose(s.fp);\n> >\n> > This is fixing the leaked file descriptor.\n> >\n> > One thing I wonder though is that we don't perform any error checking on\n> > the file in the new version. Previously, we would have died in case\n> > `write_file_buf()` failed. But now we just `fwrite()` without error\n> > checking. I don't think that \"wt-status.c\" does error checking either,\n> > so we might end up with a partially-written file without us noticing.\n> \n> Yes, the fwrite() should be protected with an error checking and\n> die() the same way as the code before. Will send a v2.\n> \n> But isn't the end result the same between preimage and postimage?\n> If the stuff appended by wt_status_* are still written without error\n> checking, we would leave a partially-written file that has the\n> default_messages and the commented hint/action but not necessarily\n> whatever we wanted to add with wt_status().\n\nAt least it would only be the status information that's missing in that\ncase, the commit message itself would be retained (or we'd die if it\nwasn't written). So we didn't have the potential to loose information\nthat is intended to end up in the final commit.\n\nPatrick\n"},{"id":"546678","messageId":"xmqqmrwdxrat.fsf@gitster.g","threadId":"65875","inReplyTo":"xmqqecht8df1.fsf@gitster.g","subject":"[PATCH v2] history: streamline message preparation and plug file stream leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-29T16:08:42Z","receivedAt":"2026-06-29T16:08:44Z","isPatch":true,"body":"An early part of fill_commit_message() function uses write_file_buf()\nto write out what was prepared in a strbuf, which is primarily meant\nfor use by callers that have their own message prepared fully and\ncalled as the last thing to flush it to the destination file.\n\nHowever, the function then opens a file stream in append mode to\nfurther write into it. It may have been understandable if this was\na later addition, but it seems it came from a single commit,\nd205234c (builtin/history: implement \"reword\" subcommand,\n2026-01-13), which is somewhat puzzling, but anyway...\n\nJust open the file stream upfront for writing, write the message\nthe function has in the strbuf, and then keep writing whatever it\nwants to write to the same open file stream.\n\nAnd do not forget to close the stream. We are about to pass the\nresulting file to an external editor, and on some systems, notably\nWindows, you are not supposed to keep a file open while expecting\nanother program to access it.\n\nDiagnosed-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n\n * Changes from v1 are two additional error checks to notice failure\n from fwrite() and fclose() to die. Interdiff appears at the end.\n\n builtin/history.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/history.c b/builtin/history.c\nindex 8dcb9a6046..365e81379b 100644\n--- a/builtin/history.c\n+++ b/builtin/history.c\n@@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo,\n \t\t \" empty message aborts the commit.\\n\");\n \tstruct wt_status s;\n \n-\tstrbuf_addstr(out, default_message);\n-\tstrbuf_addch(out, '\\n');\n-\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n-\twrite_file_buf(path, out->buf, out->len);\n-\n \twt_status_prepare(repo, &s);\n \tFREE_AND_NULL(s.branch);\n \ts.ahead_behind_flags = AHEAD_BEHIND_QUICK;\n@@ -57,14 +52,22 @@ static int fill_commit_message(struct repository *repo,\n \ts.whence = FROM_COMMIT;\n \ts.committable = 1;\n \n-\ts.fp = fopen(git_path_commit_editmsg(), \"a\");\n+\ts.fp = fopen(path, \"w\");\n \tif (!s.fp)\n-\t\treturn error_errno(_(\"could not open '%s'\"), git_path_commit_editmsg());\n+\t\treturn error_errno(_(\"could not open '%s'\"), path);\n+\n+\tstrbuf_addstr(out, default_message);\n+\tstrbuf_addch(out, '\\n');\n+\tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n+\tif (fwrite(out->buf, 1, out->len, s.fp) != out->len)\n+\t\tdie_errno(_(\"could not write to '%s'\"), path);\n \n \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n \twt_status_print(&s);\n \twt_status_collect_free_buffers(&s);\n \tstring_list_clear_func(&s.change, change_data_free);\n+\tif (fclose(s.fp))\n+\t\tdie_errno(_(\"could not write to '%s'\"), path);\n \n \tstrbuf_reset(out);\n \tif (launch_editor(path, out, NULL)) {\n\nInterdiff against v1:\n diff --git a/builtin/history.c b/builtin/history.c\n index f17ec049c0..365e81379b 100644\n --- a/builtin/history.c\n +++ b/builtin/history.c\n @@ -59,13 +59,15 @@ static int fill_commit_message(struct repository *repo,\n \tstrbuf_addstr(out, default_message);\n \tstrbuf_addch(out, '\\n');\n \tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n -\tfwrite(out->buf, 1, out->len, s.fp);\n +\tif (fwrite(out->buf, 1, out->len, s.fp) != out->len)\n +\t\tdie_errno(_(\"could not write to '%s'\"), path);\n \n \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n \twt_status_print(&s);\n \twt_status_collect_free_buffers(&s);\n \tstring_list_clear_func(&s.change, change_data_free);\n -\tfclose(s.fp);\n +\tif (fclose(s.fp))\n +\t\tdie_errno(_(\"could not write to '%s'\"), path);\n \n \tstrbuf_reset(out);\n \tif (launch_editor(path, out, NULL)) {\n-- \n2.55.0-180-gf61bfe2e0b\n\n\n"},{"id":"546776","messageId":"akO1mhi2u2PntLbt@pks.im","threadId":"65875","inReplyTo":"xmqqmrwdxrat.fsf@gitster.g","subject":"Re: [PATCH v2] history: streamline message preparation and plug file stream leak","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-30T12:24:58Z","receivedAt":"2026-06-30T12:25:07Z","isPatch":true,"body":"On Mon, Jun 29, 2026 at 09:08:42AM -0700, Junio C Hamano wrote:\n[snip]\n> * Changes from v1 are two additional error checks to notice failure\n> from fwrite() and fclose() to die. Interdiff appears at the end.\n\nTechnically speaking the first error check for fwrite() should be\nunnecessary as the errors accumulate. But it doesn't hurt, either.\n\n> Interdiff against v1:\n> diff --git a/builtin/history.c b/builtin/history.c\n> index f17ec049c0..365e81379b 100644\n> --- a/builtin/history.c\n> +++ b/builtin/history.c\n> @@ -59,13 +59,15 @@ static int fill_commit_message(struct repository *repo,\n> \tstrbuf_addstr(out, default_message);\n> \tstrbuf_addch(out, '\\n');\n> \tstrbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str);\n> -\tfwrite(out->buf, 1, out->len, s.fp);\n> +\tif (fwrite(out->buf, 1, out->len, s.fp) != out->len)\n> +\t\tdie_errno(_(\"could not write to '%s'\"), path);\n> \n> \twt_status_collect_changes_trees(&s, old_tree, new_tree);\n> \twt_status_print(&s);\n> \twt_status_collect_free_buffers(&s);\n> \tstring_list_clear_func(&s.change, change_data_free);\n> -\tfclose(s.fp);\n> +\tif (fclose(s.fp))\n> +\t\tdie_errno(_(\"could not write to '%s'\"), path);\n> \n> \tstrbuf_reset(out);\n> \tif (launch_editor(path, out, NULL)) {\n\nYup, this looks good to me. Thanks!\n\nPatrick\n"}]}