git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Andrzej Hunt" <andrzej@ahunt.org>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Lénaïc Huard" <lenaic@lhuard.fr>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Elijah Newren" <newren@gmail.com>
Subject: Re: [PATCH v2 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist
Date: Tue, 31 Aug 2021 14:47:01 +0200	[thread overview]
Message-ID: <87y28hwylq.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YPH3OOOfK9RVebqZ@coredump.intra.peff.net>


On Fri, Jul 16 2021, Jeff King wrote:

[Very late reply, just getting back to this thread]

> On Fri, Jul 16, 2021 at 09:46:33AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > I can't say I _love_ any of that, but I think it would work (and
>> > probably we'd adapt our helpers like git_config_pathname() to take a
>> > def_string. Or I guess just have a def_string_free() which can be called
>> > before writing into them).
>> >
>> > But maybe there's a better solution I'm missing.
>> 
>> Instead of: "int from_heap" in your "def_string" I think we should just
>> use "struct string_list_item". I.e. you want a void* here. Why?
>
> Yes, an equivalent way to write it is with a separate to_free buffer.
> But why would we want it to be void? And why would we want to use a
> string_list_item, which is otherwise unrelated?

We could factor "string_list_item" out into a "string_pair" or
whatever.

Sorry, I didn't mean to get into the naming/code location aspect of the
discussion, just the sensibility of using a "char */void * pair" for
these common memory management cases, v.s. your suggestion of having a
"char *, int from_heap" pair.

>> Well, tying this back to my clear() improvements for string-list.h I
>> thought a really neat solution to this was:
>> 
>>     string_list_append(list, ptr)->util = on_heap_now_we_own_it;
>>     string_list_append(list, mydup)->util = mydup;
>> 
>> I.e. by convention we store the pointer we need to free (if any) in the
>> "util" field.
>
> 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.

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.

I'm not suggesting that this handles every possible scenario, just that
having look at a lot of the code involved recently this seemed like a
neat solution for the common cases.

>> We're not in the habit of passing loose "string_list_item" around now,
>> but I don't see why we wouldn't (possibly with a change to extract that
>> bit out, so we could use it in other places).
>
> It seems unnecessarily confusing to me. It sounds like you have a struct
> which just _happens_ to have a "void *" in it you can re-use, so you
> start using it in lots of other places that are not in fact string lists
> at all. That is confusing to me on the face, but what happens when
> string_list needs a feature which requires adding more fields to it?
>
> If the point is to have a maybe-allocated string, why not make that a
> type itself? And then if we want string_list to use it, it can.

*nod*, covered above. My examples were unnecessarily confusing...

>> The neat thing about doing this is also that you're not left with every
>> API boundary needing to deal with your new "def_string", a lot of them
>> use string_list already, and hardly need to change anything, to the
>> extent that we do need to change anything having a "void *util" is a lot
>> more generally usable. You end up getting memory management for free as
>> you gain a feature to pass arbitrary data along with your items.
>
> 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.

  reply	other threads:[~2021-08-31 13:08 UTC|newest]

Thread overview: 125+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-09 14:38 UNLEAK(), leak checking in the default tests etc Ævar Arnfjörð Bjarmason
2021-06-09 17:44 ` Andrzej Hunt
2021-06-09 20:36   ` Felipe Contreras
2021-06-10 10:46   ` Jeff King
2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
2021-06-10 13:38     ` Jeff King
2021-06-10 15:32       ` Andrzej Hunt
2021-06-10 16:36         ` Jeff King
2021-06-11 15:44           ` Andrzej Hunt
2021-06-10 19:01 ` SZEDER Gábor
2021-07-14  0:11 ` [PATCH 0/4] add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 1/4] tests: " Ævar Arnfjörð Bjarmason
2021-07-14  3:23     ` Đoàn Trần Công Danh
2021-07-14  0:11   ` [PATCH 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 3/4] SANITIZE tests: fix memory leaks in t5701*, " Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 4/4] SANITIZE tests: fix leak in mailmap.c Ævar Arnfjörð Bjarmason
2021-07-14  2:19     ` Eric Sunshine
2021-07-14 17:23   ` [PATCH v2 0/4] add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 1/4] tests: " Ævar Arnfjörð Bjarmason
2021-07-14 18:42       ` Andrzej Hunt
2021-07-14 22:39         ` Ævar Arnfjörð Bjarmason
2021-07-15 21:14         ` Jeff King
2021-07-15 21:06       ` Jeff King
2021-07-16 14:46         ` Ævar Arnfjörð Bjarmason
2021-07-16 18:09           ` Jeff King
2021-07-16 18:45             ` Jeff King
2021-07-16 18:56             ` Ævar Arnfjörð Bjarmason
2021-07-16 19:22               ` Jeff King
2021-07-14 17:23     ` [PATCH v2 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist Ævar Arnfjörð Bjarmason
2021-07-14 18:57       ` Andrzej Hunt
2021-07-14 22:56         ` Ævar Arnfjörð Bjarmason
2021-07-15 21:42         ` Jeff King
2021-07-16  5:18           ` Andrzej Hunt
2021-07-16 21:20             ` Jeff King
2021-07-16  7:46           ` Ævar Arnfjörð Bjarmason
2021-07-16 21:16             ` Jeff King
2021-08-31 12:47               ` Ævar Arnfjörð Bjarmason [this message]
2021-09-01  7:53                 ` Jeff King
2021-09-01 11:45                   ` Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 3/4] SANITIZE tests: fix memory leaks in t5701*, " Ævar Arnfjörð Bjarmason
2021-07-15 17:37       ` Andrzej Hunt
2021-07-15 21:43       ` Jeff King
2021-08-31 13:46       ` [PATCH] protocol-caps.c: fix memory leak in send_info() Ævar Arnfjörð Bjarmason
2021-08-31 15:32         ` Bruno Albuquerque
2021-08-31 18:15           ` Junio C Hamano
     [not found]         ` <CAPeR6H69a_HMwWnpHzssaCm_ow=ic7AnzMdZVQJQ2ECRDaWzaA@mail.gmail.com>
2021-08-31 20:08           ` Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 4/4] SANITIZE tests: fix leak in mailmap.c Ævar Arnfjörð Bjarmason
2021-08-31 13:42       ` [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}() Ævar Arnfjörð Bjarmason
2021-08-31 16:22         ` Eric Sunshine
2021-08-31 19:38         ` Jeff King
2021-08-31 19:46           ` Junio C Hamano
2021-07-15 17:37     ` [PATCH v2 0/4] add a test mode for SANITIZE=leak, run it in CI Andrzej Hunt
2021-08-31 13:35     ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
2021-09-01  9:56       ` Jeff King
2021-09-01 10:42         ` Jeff King
2021-09-02 12:25         ` Ævar Arnfjörð Bjarmason
2021-09-03 11:13           ` Jeff King
2021-09-07 15:33       ` [PATCH v4 0/3] " Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 1/3] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 2/3] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 3/3] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-07 16:29           ` Eric Sunshine
2021-09-07 16:51           ` Jeff King
2021-09-07 16:44         ` [PATCH v4 0/3] " Jeff King
2021-09-07 18:22           ` Junio C Hamano
2021-09-07 21:30         ` [PATCH v5 " Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 1/3] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 2/3] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 3/3] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-08  4:46             ` Eric Sunshine
2021-09-16  3:56             ` [PATCH] fixup! " Carlo Marcelo Arenas Belón
2021-09-16  6:14               ` Ævar Arnfjörð Bjarmason
2021-09-08 11:02           ` [PATCH v5 0/3] " Junio C Hamano
2021-09-08 12:03             ` Ævar Arnfjörð Bjarmason
2021-09-09 23:10               ` Emily Shaffer
2021-09-16 10:48           ` [PATCH v6 0/2] " Ævar Arnfjörð Bjarmason
2021-09-16 10:48             ` [PATCH v6 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-16 10:48             ` [PATCH v6 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-19  8:03             ` [PATCH v7 0/2] " Ævar Arnfjörð Bjarmason
2021-09-19  8:03               ` [PATCH v7 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-19  8:03               ` [PATCH v7 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-22 11:17                 ` [PATCH] fixup! " Carlo Marcelo Arenas Belón
2021-09-23  1:50                   ` Ævar Arnfjörð Bjarmason
2021-09-23  9:20               ` [PATCH v8 0/2] " Ævar Arnfjörð Bjarmason
2021-09-23  9:20                 ` [PATCH v8 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-23  9:20                 ` [PATCH v8 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-11-03 22:44                   ` Re* " Junio C Hamano
2021-11-03 23:57                     ` Junio C Hamano
2021-11-04 10:06                     ` Ævar Arnfjörð Bjarmason
2021-11-16 18:31                       ` [PATCH] t0006: date_mode can leak .strftime_fmt member Ævar Arnfjörð Bjarmason
2021-11-16 19:04                         ` Junio C Hamano
2021-11-16 19:31                         ` Jeff King
2022-02-02 21:03                           ` [PATCH 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-02 21:19                               ` Ævar Arnfjörð Bjarmason
2022-02-15  3:04                               ` Junio C Hamano
2022-02-02 21:03                             ` [PATCH 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-15  2:14                               ` Junio C Hamano
2022-02-02 21:03                             ` [PATCH 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-15  0:28                               ` Junio C Hamano
2022-02-04 23:53                             ` [PATCH v2 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-14 17:25                               ` [PATCH v2 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-14 19:52                                 ` Junio C Hamano
2022-02-16  8:14                               ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-16 17:45                                 ` [PATCH v3 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Junio C Hamano
     [not found]     ` <cover-v3-0.8-00000000000-20210831T132607Z-avarab@gmail.com>
2021-08-31 13:35       ` [PATCH v3 1/8] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 2/8] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 3/8] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 4/8] tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 5/8] tests: annotate t001*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 6/8] tests: annotate t002*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 7/8] tests: annotate select t0*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 8/8] tests: annotate select t*.sh " Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y28hwylq.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=andrzej@ahunt.org \
    --cc=congdanhqx@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=lenaic@lhuard.fr \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).