Re: [PATCH 1/3] log-tree: take ownership of pointer

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Mar 12, 2024, 17:43 UTC
Message-ID: <73a4cb87-2800-4ad1-b7a2-33c6465fcc50@app.fastmail.com>
In-Reply-To: <20240312092959.GA96171@coredump.intra.peff.net>

Hi Jeff and thanks for taking a look

On Tue, Mar 12, 2024, at 10:29, Jeff King wrote:

Show 17 quoted lines

> On Thu, Mar 07, 2024 at 08:59:35PM +0100, Kristoffer Haugsbakk wrote:
>
>> 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.

Is it okay if it is done in patch 2?

> It frees the old extra_headers before reassigning it, but nobody
> cleans it up after handling the final commit.

I didn’t get any leak errors from the CI. `extra_headers` in `show_log` is populated by calling `log_write_email_headers`. Then later it is assigned to

    ctx.after_subject = extra_headers;

Then `ctx.after_subject is freed later

    free((char *)ctx.after_subject);

Am I missing something?

> 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.

Okay, I’ll do that.

Thanks