Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 26, 2017, 10:16 UTC
Message-ID: <CAN0heSq8RdH5vWFgq1UvJOfWerMJSZwhV4FMCjvA=XUqu2OQQQ@mail.gmail.com>
In-Reply-To: <xmqq8ti7s6ph.fsf@gitster.mtv.corp.google.com>

On 25 August 2017 at 23:00, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
>> files_transaction_prepare() and the functions it calls add strings to a
>> string list without duplicating them, i.e., we keep the original raw
>> pointers we were given. That is "ok", since we keep them only for a
>> short-enough time, but we end up leaking some of them.
>
> Sorry, but I do not understand this statement.  If affected_refnames
> string list borrows strings from other structures who own them, and
> none of these strings are freed by clearing affected_refnames list,
> that is not "leaking"---we didn't acquire the ownership, so it is
> not our job to free them in the first place.  Among the original
> owners of strings we borrow from, some may not properly free, in
> which case that is a leak.
>
> What problem are you solving?

Sorry. Maybe this explains my intentions better:

    In lock_ref_for_update(), we populate a strbuf "referent" through
    lock_raw_ref(). If we don't have a symref, we don't use "referent"
    for anything (and it won't have allocated any memory). Otherwise, we
    hand over referent.buf to someone who uses it immediately
    (refs_read_ref_full) or to someone who holds on to the pointer
    (split_symref_update ends up adding it to a string list). Therefore,
    at the end of lock_ref_for_update() we can't unconditionally release
    the strbuf, so we end up leaking it.

    We could release the strbuf when we know that it's safe (possibly
    also only when we know that it's needed). Instead, in preparation
    for the next patch, make the string list not hold on to the raw
    pointers, i.e., make it duplicate the strings on insertion and
    manage its own resources.

Of course, the pointer-keeping and free-avoidance might be by design and/or wanted, e.g., to avoid excessive mallocing and freeing. I admit to not knowing what is a realistic number of iterations in the loop that calls lock_ref_for_update, i.e., how severe this leak might be. Maybe the "backend" nature of this code does not necessarily imply "this could be called any number of times throughout the process' lifetime".