Re: [PATCH 2/2] format-patch: free elements of rev.ref_message_ids list
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 19, 2023, 01:32 UTC
- Message-ID
- <xmqqttw9qf6p.fsf@gitster.g>
- In-Reply-To
- <20230519000543.GB1975194@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> When we are showing multiple patches with format-patch, we have to > repeatedly overwrite the rev.message_id field. We take care to avoid > leaking the old value by either freeing it, or adding it to > ref_message_ids, a string list of ids to reference in subsequent > messages. > > But unfortunately we do leak the value via that string list. We try > to clear the string list, courtesy of 89f45cf4eb (format-patch: don't > leak "extra_headers" or "ref_message_ids", 2022-04-13). But since it was > initialized as "nodup", the string list doesn't realize it owns the > strings, and it leaks them. > > We have two options here: > > 1. Continue to init with "nodup", but then tweak the value of > ref_message_ids.strdup_strings just before clearing. > > 2. Init with "dup", but use "append_nodup" when transferring ownership > of strings to the list. Clearing just works. > > I picked the second here, as I think it calls attention to the tricky > part (transferring ownership via the nodup call).
This is pretty much what I had in mind wheh I wrote the log message for the follow-up "Yikes, for now let's mark the test no longer sanitizer clean" patch. Very pleased to see a clean-up I punted materialize while I was looking the other way ;-)
Show 5 quoted lines
> There's one other related fix we have to do, though. We also insert the > result of clean_message_id() into the list. This _sometimes_ allocates > and sometimes does not, depending on whether we have to remove cruft > from the end of the string. Let's teach it to consistently return an > allocated string, so that the caller knows it must be freed.
Yes!
Thanks.