On Tue, Jun 04, 2024 at 11:36:52AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > 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.
>
> Yeah, that is what had me puzzled ;-)
Interestingly, SANITIZE=address does find the leak. We usually disable
its leak-checking because the leaks get in the way of seeing the more
important output.
It's a little concerning that SANITIZE=leak doesn't catch this case.
Just a wild guess, but I can imagine it might have to do with how LSan
starts its reachability analysis, and that ASan is a bit more careful
about invalidating the now-bogus stack memory. I dunno.
At any rate, in the long run I hope we'll be able to ditch LSan
entirely, once the test suite is leak free. Then we can just let the
regular ASan job report the leaks it finds and a leak gets the same
severity as any other sanitizer error (from the perspective of CI).
> > 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).
>
> Thanks.
I see you applied this on top of t1517 to fix the broken CI. That makes
sense (though did give me a moment of head-scratching when I tried to
reproduce my ASan findings using next!).
I do think it's the wrong fix in the long term, and we'd want a patch
like this on top of the merge of jc/t1517-more and ps/no-writable-strings.
-- >8 --
Subject: [PATCH] imap-send: free all_msgs strbuf in "out" label
We read stdin into a strbuf, but most code paths never release it,
causing a leak (albeit a minor one, as we leak only when exiting from
the main function of the program).
Commit 56f4f4a29d (imap-send: minimum leakfix, 2024-06-04) did the
minimum to plug the one instance we see in the test suite, when we read
an empty input. But it was sufficient only because aside from this noop
invocation, we don't test imap-send at all!
The right spot to free is in the "out" label, which is hit by all code
paths before leaving the function. We couldn't do that in 56f4f4a29d
because there was no unified exit path. That came separately in
3aca5f7fb0 (imap-send: fix leaking memory in `imap_server_conf`,
2024-06-04), which cleaned up many other leaks (but not this one).
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
An alternative universe version of this one just fixes the leak in the
"out" label without removing the call added by jc/t1517-more, and could
be applied directly onto ps/no-writable-strings. But then we'd want to
remember to remove the redundant one once the two topics are merged.
This one is a little hassle to apply, but it feels worth it to capture
the right fix while we're thinking about it (though of course I still
dream of nuking imap-send.c from orbit).
imap-send.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/imap-send.c b/imap-send.c
index e602edc4be..01404e5047 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1555,7 +1555,6 @@ int cmd_main(int argc, const char **argv)
}
if (all_msgs.len == 0) {
- strbuf_release(&all_msgs);
fprintf(stderr, "nothing to send\n");
ret = 1;
goto out;
@@ -1586,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;
}
--
2.45.2.807.g3b5fadc4da
