Volume XXII, number 280Wednesday, October 7, 2026Latest message 49 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchhistory: streamline message preparation and plug file stream leak

6 messages between Jun 26, 2026 and Jun 30, 2026, from Junio C Hamano, Patrick Steinhardt.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Junio C HamanoJun 26, 2026, 16:38 UTC on lore

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.

Diagnosed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * As the initial one was written and sent as "how about doing a bit
   more thorough job while we are at it?" response to a posted patch
   found in <pull.2158.git.1782412427801.gitgitgadget@gmail.com>,
   this is a tested and merge-ready cersion that I consider "v1".
 builtin/history.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to builtin/history.c +8 −7
diff --git a/builtin/history.c b/builtin/history.c
index 8dcb9a6046..f17ec049c0 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -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)) {

-- 
2.55.0-rc2-177-gc9430f6415
Patrick SteinhardtJun 29, 2026, 06:33 UTC in reply to Junio C Hamano on lore

Re: [PATCH] history: streamline message preparation and plug file stream leak

On Fri, Jun 26, 2026 at 09:38:42AM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> diff --git a/builtin/history.c b/builtin/history.c
> index 8dcb9a6046..f17ec049c0 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -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");

Here we reuse the local `path` variable, which already carries the result of `git_path_commit_editmsg()`.

> +	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);
Likewise.
Show 10 quoted lines
> +	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);
This is fixing the leaked file descriptor.

One thing I wonder though is that we don't perform any error checking on the file in the new version. Previously, we would have died in case `write_file_buf()` failed. But now we just `fwrite()` without error checking. I don't think that "wt-status.c" does error checking either, so we might end up with a partially-written file without us noticing.

Thanks!
Patrick
Junio C HamanoJun 29, 2026, 15:21 UTC in reply to Patrick Steinhardt on lore

Re: [PATCH] history: streamline message preparation and plug file stream leak

Patrick Steinhardt <ps@pks.im> writes:
Show 18 quoted lines
>> +	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);
>
> This is fixing the leaked file descriptor.
>
> One thing I wonder though is that we don't perform any error checking on
> the file in the new version. Previously, we would have died in case
> `write_file_buf()` failed. But now we just `fwrite()` without error
> checking. I don't think that "wt-status.c" does error checking either,
> so we might end up with a partially-written file without us noticing.

Yes, the fwrite() should be protected with an error checking and die() the same way as the code before. Will send a v2.

But isn't the end result the same between preimage and postimage? If the stuff appended by wt_status_* are still written without error checking, we would leave a partially-written file that has the default_messages and the commented hint/action but not necessarily whatever we wanted to add with wt_status().

Patrick SteinhardtJun 29, 2026, 16:04 UTC in reply to Junio C Hamano on lore

Re: [PATCH] history: streamline message preparation and plug file stream leak

On Mon, Jun 29, 2026 at 08:21:06AM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> +	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);
> >
> > This is fixing the leaked file descriptor.
> >
> > One thing I wonder though is that we don't perform any error checking on
> > the file in the new version. Previously, we would have died in case
> > `write_file_buf()` failed. But now we just `fwrite()` without error
> > checking. I don't think that "wt-status.c" does error checking either,
> > so we might end up with a partially-written file without us noticing.
> 
> Yes, the fwrite() should be protected with an error checking and
> die() the same way as the code before.  Will send a v2.
> 
> But isn't the end result the same between preimage and postimage?
> If the stuff appended by wt_status_* are still written without error
> checking, we would leave a partially-written file that has the
> default_messages and the commented hint/action but not necessarily
> whatever we wanted to add with wt_status().

At least it would only be the status information that's missing in that case, the commit message itself would be retained (or we'd die if it wasn't written). So we didn't have the potential to loose information that is intended to end up in the final commit.

Patrick
Junio C HamanoJun 29, 2026, 16:08 UTC in reply to Junio C Hamano on lore

[PATCH v2] history: streamline message preparation and plug file stream leak

An early part of fill_commit_message() 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.

Diagnosed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Changes from v1 are two additional error checks to notice failure
   from fwrite() and fclose() to die.  Interdiff appears at the end.
 builtin/history.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to builtin/history.c +10 −7
diff --git a/builtin/history.c b/builtin/history.c
index 8dcb9a6046..365e81379b 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -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,22 @@ 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);
+	if (fwrite(out->buf, 1, out->len, s.fp) != out->len)
+		die_errno(_("could not write to '%s'"), path);
 
 	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);
+	if (fclose(s.fp))
+		die_errno(_("could not write to '%s'"), path);
 
 	strbuf_reset(out);
 	if (launch_editor(path, out, NULL)) {

Interdiff against v1:
  diff --git a/builtin/history.c b/builtin/history.c
  index f17ec049c0..365e81379b 100644
  --- a/builtin/history.c
  +++ b/builtin/history.c
  @@ -59,13 +59,15 @@ static int fill_commit_message(struct repository *repo,
   	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);
  +	if (fwrite(out->buf, 1, out->len, s.fp) != out->len)
  +		die_errno(_("could not write to '%s'"), path);
   
   	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);
  +	if (fclose(s.fp))
  +		die_errno(_("could not write to '%s'"), path);
   
   	strbuf_reset(out);
   	if (launch_editor(path, out, NULL)) {

-- 
2.55.0-180-gf61bfe2e0b
Patrick SteinhardtJun 30, 2026, 12:24 UTC in reply to Junio C Hamano on lore

Re: [PATCH v2] history: streamline message preparation and plug file stream leak

On Mon, Jun 29, 2026 at 09:08:42AM -0700, Junio C Hamano wrote: [snip]

>  * Changes from v1 are two additional error checks to notice failure
>    from fwrite() and fclose() to die.  Interdiff appears at the end.

Technically speaking the first error check for fwrite() should be unnecessary as the errors accumulate. But it doesn't hurt, either.

Show 23 quoted lines
> Interdiff against v1:
>   diff --git a/builtin/history.c b/builtin/history.c
>   index f17ec049c0..365e81379b 100644
>   --- a/builtin/history.c
>   +++ b/builtin/history.c
>   @@ -59,13 +59,15 @@ static int fill_commit_message(struct repository *repo,
>    	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);
>   +	if (fwrite(out->buf, 1, out->len, s.fp) != out->len)
>   +		die_errno(_("could not write to '%s'"), path);
>    
>    	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);
>   +	if (fclose(s.fp))
>   +		die_errno(_("could not write to '%s'"), path);
>    
>    	strbuf_reset(out);
>    	if (launch_editor(path, out, NULL)) {
Yup, this looks good to me. Thanks!
Patrick

Back to recent threads