Re: What's cooking in git.git (Jun 2024, #01; Mon, 3)

[Jeff King <peff@peff.net>]

On Mon, Jun 03, 2024 at 11:16:11PM -0700, Junio C Hamano wrote:

> * jc/t1517-more (2024-05-31) 1 commit
>   (merged to 'next' on 2024-06-03 at 10b71e2a60)
>  + t1517: more coverage for commands that work without repository
> 
>  "smoke tests" to ensure git commands that are designed to run
>  outside repositories do work.
> 
>  Will merge to 'master'.
>  source: <xmqqwmnajrrk.fsf@gitster.g>

This one seems to fail the CI leak jobs (I noticed it on next, but I
think even the tip of the feature branch fails).

One possible fix is this:

	diff --git a/imap-send.c b/imap-send.c
	index a5d1510180..26d5909e79 100644
	--- a/imap-send.c
	+++ b/imap-send.c
	@@ -1539,16 +1539,17 @@ int cmd_main(int argc, const char **argv)
	 	/* read the messages */
	 	if (strbuf_read(&all_msgs, 0, 0) < 0) {
	 		error_errno(_("could not read from stdin"));
	 		return 1;
	 	}
	 
	 	if (all_msgs.len == 0) {
	 		fprintf(stderr, "nothing to send\n");
	+		strbuf_release(&all_msgs);
	 		return 1;
	 	}
	 
	 	total = count_messages(&all_msgs);
	 	if (!total) {
	 		fprintf(stderr, "no messages to send\n");
	 		return 1;
	 	}

But I wonder if strbuf_read() should handle the allocation itself when
it does a 0-byte read. We already do so for an error return (so the
"could not read from stdin" path above is actually OK).

Maybe a moot point, though. I think we _always_ leak the all_msgs
strbuf, so probably the whole function needs more unified cleanup. It
looks like Patrick's 11637fc740 (imap-send: fix leaking memory in
`imap_server_conf`, 2024-06-03) does that refactoring (but isn't yet in
'next'). Once ps/no-writable-strings is merged, we should be able to
just do:

diff --git a/imap-send.c b/imap-send.c
index da3e7ec17e..01404e5047 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1585,5 +1585,6 @@ int cmd_main(int argc, const char **argv)
 	free(server.user);
 	free(server.pass);
 	free(server.auth_method);
+	strbuf_release(&all_msgs);
 	return ret;
 }

on top. Weirdly, with ps/no-writable-strings merged (but without the fix
above applied yet), SANITIZE=leak does not seem to find the leak
anymore! Even though I can confirm in a debugger or by printing the
strbuf's fields that it is still there. So that's...odd. But whatever is
going on with LSan, the fix above is the right thing.

-Peff