Hi Jeff and thanks for taking a look

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

On Thu, Mar 07, 2024 at 08:59:35PM +0100, Kristoffer Haugsbakk wrote:

quoted

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