Re: [PATCH] leak tests: add an interface to the LSAN_OPTIONS "suppressions"

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 26, 2021, 21:30 UTC
Message-ID: <YXhzh+pW5KvU7MEA@nand.local>
In-Reply-To: <YXhvAQgxKpq65qcg@coredump.intra.peff.net>

On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:

Show 13 quoted lines

> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
>
> > So this all feels like a bug in ASan to me. I'm curious if it works on
> > your system, but in the meantime I think the best path forward is to
> > drop the last patch of my original series (the one with the three
> > UNLEAK() calls) and to avoid relying on this patch for the time being.
>
> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
> for all the reasons we introduced UNLEAK() in the first place:
>
>   - it keeps the annotations near the code. Yes, that creates conflicts
>     when the code is changed (or the leak is actually fixed), but that's
>     a feature. It keeps them from going stale.

I agree completely. I noted as much in my message here:

    https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/

but Ævar made it sound like his work would be made much easier without the conflict. Since I'm not in any kind of rush to make t5319 leak-free, I figured that queueing the parts of that series that wouldn't conflict with Ævar's ongoing work would be a net-positive.

Show 5 quoted lines

>   - leak-checkers only know where things are allocated, not who is
>     supposed to own them. So you're often annotating the wrong thing;
>     it's not a strdup() call which is buggy and leaking, but the
>     function five layers up the stack which was supposed to take
>     ownership and didn't.

TBH, I find this much more compelling. Either way, I don't feel strongly enough to deviate from v2 much, and I don't want to create more work for Ævar along the way.

Thanks, Taylor