From: "Martin Ågren" <martin.agren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames
Date: Sat, 26 Aug 2017 12:16:44 +0200 [thread overview]
Message-ID: <CAN0heSq8RdH5vWFgq1UvJOfWerMJSZwhV4FMCjvA=XUqu2OQQQ@mail.gmail.com> (raw)
In-Reply-To: <xmqq8ti7s6ph.fsf@gitster.mtv.corp.google.com>
On 25 August 2017 at 23:00, Junio C Hamano <gitster@pobox.com> wrote:
> 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".
next prev parent reply other threads:[~2017-08-26 10:16 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-25 18:49 [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames Martin Ågren
2017-08-25 18:49 ` [PATCH 2/2] refs/files-backend: fix memory leak in lock_ref_for_update Martin Ågren
2017-08-25 21:00 ` [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames Junio C Hamano
2017-08-26 10:16 ` Martin Ågren [this message]
2017-08-28 8:06 ` Michael Haggerty
2017-08-28 10:09 ` Martin Ågren
2017-08-28 20:32 ` [PATCH v2 1/2] refs/files-backend: add longer-scoped copy of string to list Martin Ågren
2017-08-29 8:33 ` Michael Haggerty
2017-08-28 20:32 ` [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update Martin Ågren
2017-08-29 8:39 ` Michael Haggerty
2017-08-29 10:41 ` Martin Ågren
2017-08-29 17:18 ` [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list Martin Ågren
2017-08-30 2:52 ` Michael Haggerty
2017-08-30 18:02 ` Martin Ågren
2017-09-05 10:02 ` Junio C Hamano
2017-09-05 17:24 ` Martin Ågren
2017-09-05 20:36 ` Jeff King
2017-09-05 21:26 ` Junio C Hamano
2017-09-06 18:12 ` Martin Ågren
2017-09-06 19:52 ` Junio C Hamano
2017-09-06 23:45 ` Jeff King
2017-09-09 6:57 ` [PATCH v4 0/4] Rerolling ma/split-symref-update-fix Martin Ågren
2017-09-09 6:57 ` [PATCH v4 1/4] refs/files-backend: add longer-scoped copy of string to list Martin Ågren
2017-09-09 6:57 ` [PATCH v4 2/4] refs/files-backend: fix memory leak in lock_ref_for_update Martin Ågren
2017-09-09 6:57 ` [PATCH v4 3/4] refs/files-backend: correct return value " Martin Ågren
2017-09-09 6:57 ` [PATCH v4 4/4] refs/files-backend: add `refname`, not "HEAD", to list Martin Ågren
2017-09-09 10:47 ` [PATCH v4 0/4] Rerolling ma/split-symref-update-fix Jeff King
2017-09-05 8:45 ` [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list Jeff King
2017-09-05 9:03 ` Michael Haggerty
2017-09-05 9:04 ` Jeff King
2017-08-29 17:18 ` [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update Martin Ågren
2017-09-05 8:47 ` Jeff King
2017-09-05 17:28 ` Martin Ågren
2017-08-29 17:18 ` [PATCH v3 3/3] refs/files-backend: correct return value " Martin Ågren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN0heSq8RdH5vWFgq1UvJOfWerMJSZwhV4FMCjvA=XUqu2OQQQ@mail.gmail.com' \
--to=martin.agren@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).