From: "Martin Ågren" <firstname.lastname@example.org> To: Jeff King <email@example.com> Cc: Git Mailing List <firstname.lastname@example.org>, "brian m . carlson" <email@example.com> Subject: Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format` Date: Fri, 25 Jan 2019 20:24:35 +0100 [thread overview] Message-ID: <CAN0heSoNvTVfC6A8fFK83u4TBX3sLaTJ_NqKwkCZORiCKdVwcA@mail.gmail.com> (raw) In-Reply-To: <20190123055704.GA19601@sigill.intra.peff.net> On Wed, 23 Jan 2019 at 06:57, Jeff King <firstname.lastname@example.org> wrote: > > On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote: > > > Call `clear_...()` at the start of `read_...()` instead of just zeroing > > the struct, since we sometimes enter the function multiple times. This > > means that it is important to initialize the struct before calling > > `read_...()`, so document that. > > This part is a little counter-intuitive to me. Is anybody ever going to > pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT? I do update all users in git.git, but yeah, out-of-tree users and in-flight topics would segfault. > If so, might it be kinder for read_...() to not assume anything about > the incoming struct, and initialize it from scratch? I.e., not to use > clear() but just do the initialization step? I have some vague memory from going down that route and giving up. Now that I'm looking at it again, I think we can at least try to do something. We can make sure that "external" users that call into setup.c are fine (they'll leak, but won't crash). Out-of-tree users inside setup.c will still be able to trip on this. I don't have much spare time over the next few days, but I'll get to this. Or we could accept that we may leak when we end up calling `read()` multiple times (I could catch all leaks now, but new ones might sneak in after that) and come back to this after X months, when we can perhaps afford to be a bit more aggressive. I guess we could just rename the struct to have the compiler catch out-of-tree users... > A caller which calls read_() multiple times would presumably have an > intervening clear (either their own, or the one done on an error return > from the read function). > > Other than that minor nit, I like the overall shape of this. Thank you. Martin
next prev parent reply other threads:[~2019-01-25 19:24 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-18 7:25 [PATCH 0/3] setup: add `clear_repository_format()` Martin Ågren 2018-12-18 7:25 ` [PATCH 1/3] setup: drop return value from `read_repository_format()` Martin Ågren 2018-12-19 15:27 ` Jeff King 2018-12-19 21:42 ` Martin Ågren 2018-12-20 0:17 ` brian m. carlson 2018-12-20 2:52 ` Jeff King 2018-12-20 3:45 ` brian m. carlson 2018-12-20 14:53 ` Jeff King 2018-12-18 7:25 ` [PATCH 2/3] setup: do not use invalid `repository_format` Martin Ågren 2018-12-19 0:18 ` brian m. carlson 2018-12-19 21:43 ` Martin Ågren 2018-12-19 15:38 ` Jeff King 2018-12-19 21:46 ` Martin Ågren 2018-12-19 23:17 ` Jeff King 2018-12-20 0:21 ` brian m. carlson 2018-12-18 7:25 ` [PATCH 3/3] setup: add `clear_repository_format()` Martin Ågren 2018-12-19 15:48 ` Jeff King 2018-12-19 21:49 ` Martin Ågren 2019-01-14 18:34 ` [PATCH v2 0/3] " Martin Ågren 2019-01-14 18:34 ` [PATCH v2 1/3] setup: free old value before setting `work_tree` Martin Ågren 2019-01-14 18:34 ` [PATCH v2 2/3] setup: do not use invalid `repository_format` Martin Ågren 2019-01-15 19:31 ` Jeff King 2019-01-17 6:31 ` Martin Ågren 2019-01-22 7:07 ` Jeff King 2019-01-22 13:34 ` Martin Ågren 2019-01-22 21:45 ` [PATCH v3 0/2] setup: fix memory leaks with `struct repository_format` Martin Ågren 2019-01-22 21:45 ` [PATCH v3 1/2] setup: free old value before setting `work_tree` Martin Ågren 2019-01-22 21:45 ` [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren 2019-01-23 5:57 ` Jeff King 2019-01-24 0:14 ` brian m. carlson 2019-01-25 19:25 ` Martin Ågren 2019-01-25 19:24 ` Martin Ågren [this message] 2019-01-25 19:51 ` Jeff King 2019-02-25 19:21 ` Martin Ågren 2019-02-26 17:46 ` Jeff King 2019-02-28 20:36 ` [PATCH v4 0/2] " Martin Ågren 2019-02-28 20:36 ` [PATCH v4 1/2] setup: free old value before setting `work_tree` Martin Ågren 2019-02-28 20:36 ` [PATCH v4 2/2] setup: fix memory leaks with `struct repository_format` Martin Ågren 2019-03-06 4:56 ` [PATCH v4 0/2] " Jeff King 2019-01-14 18:34 ` [PATCH v2 3/3] setup: add `clear_repository_format()` Martin Ågren
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=CAN0heSoNvTVfC6A8fFK83u4TBX3sLaTJ_NqKwkCZORiCKdVwcA@mail.gmail.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`' \ /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
Code repositories for project(s) associated with this 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).