Re: [PATCH 1/3] log-tree: take ownership of pointer
- From
Jeff King <peff@peff.net>
- Date
- Mar 12, 2024, 09:29 UTC
- Message-ID
- <20240312092959.GA96171@coredump.intra.peff.net>
- In-Reply-To
- <3b12a8cf393b6d8f0877fd7d87173c565d7d5a90.1709841147.git.code@khaugsbakk.name>
On Thu, Mar 07, 2024 at 08:59:35PM +0100, Kristoffer Haugsbakk wrote:
Show 10 quoted lines
> The MIME header handling started using string buffers in > d50b69b868d (log_write_email_headers: use strbufs, 2018-05-18). The > subject buffer is given to `extra_headers` without that variable taking > ownership; the commit “punts on that ownership” (in general, not just > for this buffer). > > In an upcoming commit we will first assign `extra_headers` to the owned > pointer from another `strbuf`. In turn we need this variable to always > contain an owned pointer so that we can free it in the calling > function.
Hmm, OK. This patch by itself introduces a memory leak. It would be nice if we could couple it with the matching free() so that we can see that the issue is fixed. It sounds like your patch 2 is going to introduce such a free, but I'm not sure it's complete. It frees the old extra_headers before reassigning it, but nobody cleans it up after handling the final commit.
We should also drop the "static" from subject_buffer, if it is no longer needed. Likewise, any strings that start owning memory (here or in patch 2) should probably drop their "const". That makes the ownership more clear, and avoids ugly casts when freeing.
-Peff