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