Re: [PATCH v2 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist

On Tue, Aug 31, 2021 at 02:47:01PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

That works, but now "util" is not available for all the _other_ uses for
which it was intended. And if we're not using it for those other uses,
then why does it need to exist at all? If we are only using it to hold
the allocated string pointer, then shouldn't it be "char *to_free"?

Because having it be "char *" doesn't cover the common case of
e.g. getting an already allocated "struct something *" which contains
your string, setting the "string" in "struct string_list_item" to some
string in that struct, and the "util" to the struct itself, as we now
own it and want to free() it later in its entirety.

OK. I buy that storing a void pointer makes it more flexible. I'm not
altogether convinced this pattern is especially common, but it's not any
harder to work with than a "need_to_free" flag, so there's no reason not
to do that (and to be fair, I didn't look around for possible uses of
the pattern; it's just not one I think of as common off the top of my
head).

That and the even more common case I mentioned upthread of wanting to
ferry around the truncated version of some char *, but still wanting to
account for the original for an eventual free().

But yes, if you want to account for freeing that data *and* have util
set to something else you'll need to have e.g. your own wrapper struct
and your own string_list_clear_func() callback.

But stuffing it into the util field of string_list really feels like a
stretch, and something that would make existing string_list use painful.
There are tons of cases where util points to some totally unrelated (in
terms of memory ownership) item. I'd venture to say most cases where
string_list_clear() is called without free_util would count here.

quoted

I don't think most interfaces take a string_list_item now, so wouldn't
they similarly need to be changed? Though the point is that all of these
degrade to a regular C-string, so when you are just passing the value
(and not ownership), you would just dereference at that point.

Sure, just like things would need to be changed to handle your proposed
"struct def_string".

By piggy-backing on an already used struct in our codebase we can get a
lot of that memory management pretty much for free without much
churn.

If you squint and pretend that "struct string_list_item" isn't called
something to do with that particular collections API (but it would make
use of it) then we've already set up most of the scaffolding and
management for this.

It's that squinting that bothers me. Sure, it's _kinda_ similar. And I
don't have any problem with some kind of struct that says "this is a
string, and when you are done with it, this is how you free it". And I
don't have any problem with building the "dup" version of string_list
with that struct as a primitive. But it seems to me to be orthogonal
from the "util" pointer of a string_list, which is about creating a
mapping from the string to some other thing (which may or may not
contain the string, and may or may not be owned).

TBH, I have always found the "util" field of string_list a bit ugly (and
really most of string_list). I think most cases would be better off with
a different data structure (a set or a hash table), but we didn't have
convenient versions of those for a long time. I don't mind seeing
conversions of string_list to other data structures. But that seems to
be working against using string_list's string struct in more places.

-Peff