Re: What's cooking in git.git (Jun 2024, #01; Mon, 3)
- From
Jeff King <peff@peff.net>
- Date
- Jun 4, 2024, 08:52 UTC
- Message-ID
- <20240604085200.GA1147131@coredump.intra.peff.net>
- In-Reply-To
- <xmqqo78h1bdg.fsf@gitster.g>
On Mon, Jun 03, 2024 at 11:16:11PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> * 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