history: close COMMIT_EDITMSG before launching the editor
PATCH 1/1 history: close COMMIT_EDITMSG before launching the editor
From: Johannes Schindelin <[email protected]>
The git history reword and git history fixup subcommands prepare the commit message by writing it to COMMIT_EDITMSG and then opening that same file a second time, in append mode, through wt_status's fp field to append the status information. That second handle is never closed before launch_editor() runs, so the editor is started while git still holds the file open.
Everywhere this leaks a file descriptor, but on Windows it is outright
broken: a process cannot replace a file that another process keeps open, so an editor that rewrites COMMIT_EDITMSG by creating a fresh file in its place fails. This surfaced while running Git for Windows' test suite with BusyBox' ash as the POSIX shell: the fake editor's cp message "$1" aborts with "cp: can't create '.../COMMIT_EDITMSG': File exists" (MSYS2's coreutils cp hides the problem via its POSIX unlink emulation, BusyBox' native cp does not), making t3451-history-reword and t3453-history-fixup fail wholesale.
Close the handle once the status has been written, before handing the file off to the editor.
---
history: close COMMIT_EDITMSG before launching the editor
I noticed this problem while trying to whip MinGit-BusyBox into a better shape during the -rc phase. Technically, this is not a fix for a regression during the v2.55.0 period, but I figured it'd be better to send it now anyway than to forget about sending it after v2.55.0 is released.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2158%2Fdscho%2Ffix-fd-leak-in-history-reword-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2158/dscho/fix-fd-leak-in-history-reword-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2158
builtin/history.c | 8 ++++++++ 1 file changed, 8 insertions(+)
builtin/history.c +8 -0
@@ -74,6 +74,14 @@ static int fill_commit_message(struct repository *repo,wt_status_collect_free_buffers (&s);string_list_clear_func (&s.change, change_data_free); +/* +* Close the handle before launching the editor: on Windows an open +* handle would prevent the editor from replacing the file (e.g. +* BusyBox' `ash` cannot overwrite a file that another process keeps +* open), and leaving it open leaks the descriptor everywhere else. +*/ +fclose (s.fp); +strbuf_reset (out);if (launch_editor (path, out,NULL )) {fprintf (stderr,_ ("Aborting commit as launching the editor failed.\n" ));
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Signature · 1 line
gitgitgadget
Re* [PATCH] history: close COMMIT_EDITMSG before launching the editor
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
Close the handle once the status has been written, before handing the file off to the editor.
Assisted-by Opus 4.8
Do we even need this?
Quoted 26 lines
Signed-off-by Johannes Schindelin <[email protected]>---
history: closeCOMMIT_EDITMSGbefore launching the editorI noticed this problem while trying to whip MinGit-BusyBox into a better shape during the -rc phase. Technically, this is not a fix for a regression during the v2.55.0 period, but I figured it'd be better to send it now anyway than to forget about sending it after v2.55.0 is released.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2158%2Fdscho%2Ffix-fd-leak-in-history-reword-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2158/dscho/fix-fd-leak-in-history-reword-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2158builtin/history.c | 8 ++++++++ 1 file changed, 8 insertions(+)builtin/history.c +1
-0@@ -74,6 +74,14 @@ static int fill_commit_message(struct repository *repo,wt_status_collect_free_buffers (&s);string_list_clear_func (&s.change, change_data_free); +/*
builtin/history.c static int fill_commit_message(struct repository *repo, · line 87
Quoted 10 lines
builtin/history.c +7
-0+* Close the handle before launching the editor: on Windows an open +* handle would prevent the editor from replacing the file (e.g. +* BusyBox' `ash` cannot overwrite a file that another process keeps +* open), and leaving it open leaks the descriptor everywhere else. +*/ +fclose (s.fp); +strbuf_reset (out);if (launch_editor (path, out,NULL )) {fprintf (stderr,_ ("Aborting commit as launching the editor failed.\n" ));
The function is extremely sloppy beyond words X-<. Thanks for taking the first step to clean it up.
* It calls git_path_commit_editmsg() to obtain a constant pathname into "const char *path", but then the part that leads to this file stream leak does not even use that "path" constant. It makes two independent calls to git_path_commit_editmsg()!
* The function first calls write_file_buf() to the file, which is a convenience function when you have something you need to write upfront and just want to write it and be done with it.
* And then the unclosed file stream you just fixed.
What is surprising is that all of this was created in a single commit. I suspected that this part that does fopen() to leak the file stream was a later addition than the initial write_file_buf(), which should have been critiqued with "once you want to do your custom writing that is more than "I have this block of memory, write it into file", you should rewrite write_file_buf() call and roll it into your own custom writing", but that is not the case.
So taking the opportunity to clean things up, how about doing it this way intead?
----- >8 ---------- >8 ---------- >8 -----
Subject: [PATCH] history: streamline message preparation and plug file stream leak
An early part of fill_commit_mmessage() function uses write_file_buf() to write out what was prepared in a strbuf, which is primarily meant for use by callers that have their own message prepared fully and called as the last thing to flush it to the destination file.
However, the function then opens a file stream in append mode to further write into it. It may have been understandable if this was a later addition, but it seems it came from a single commit, d205234c (builtin/history: implement "reword" subcommand, 2026-01-13), which is somewhat puzzling, but anyway...
Just open the file stream upfront for writing, write the message the function has in the strbuf, and then keep writing whatever it wants to write to the same open file stream.
And do not forget to close the stream. We are about to pass the resulting file to an external editor, and on some systems, notably Windows, you are not supposed to keep a file open while expecting another program to access it.
--- builtin/history.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
builtin/history.c +8 -7
@@ -41,11 +41,6 @@ static int fill_commit_message(struct repository *repo, " empty message aborts the commit.\n");struct wt_status s; -strbuf_addstr (out, default_message); -strbuf_addch (out,'\n' ); -strbuf_commented_addf (out, comment_line_str, hint, action, comment_line_str); -write_file_buf (path, out->buf, out->len); -wt_status_prepare (repo, &s);FREE_AND_NULL (s.branch); s.ahead_behind_flags =AHEAD_BEHIND_QUICK ;
@@ -57,14 +52,20 @@ static int fill_commit_message(struct repository *repo, s.whence =FROM_COMMIT ; s.committable =1 ; - s.fp =fopen (git_path_commit_editmsg (),"a" ); + s.fp =fopen (path,"w" );if (!s.fp) -return error_errno (_ ("could not open '%s'" ),git_path_commit_editmsg ()); +return error_errno (_ ("could not open '%s'" ), path); + +strbuf_addstr (out, default_message); +strbuf_addch (out,'\n' ); +strbuf_commented_addf (out, comment_line_str, hint, action, comment_line_str); +fwrite (out.buf,1 , out.len, s.fp);wt_status_collect_changes_trees (&s, old_tree, new_tree);wt_status_print (&s);wt_status_collect_free_buffers (&s);string_list_clear_func (&s.change, change_data_free); +fclose (s.fp);strbuf_reset (out);if (launch_editor (path, out,NULL )) {
Signature · 1 line
2.55.0-rc2-165-g3249676ba5
Re: Re* [PATCH] history: close COMMIT_EDITMSG before launching the editor
Junio C Hamano <[email protected]> writes:
Of course, this should be ...
Quoted 1 line
builtin/history.c +1
-0+fwrite (out.buf,1 , out.len, s.fp);
...
fwrite(out->buf, 1, out->len, s.fp);