From: Matheus Tavares <matheus.bernardino@usp.br> To: git@vger.kernel.org Cc: phil.hord@gmail.com, dstolee@microsoft.com, jonathantanmy@google.com, stefanbeller@gmail.com Subject: [PATCH 2/2] packfile: fix memory leak in add_delta_base_cache() Date: Mon, 28 Sep 2020 13:50:35 -0300 [thread overview] Message-ID: <5b6e3019e08c6bccdee29018e99b0c6933fe05e0.1601311803.git.matheus.bernardino@usp.br> (raw) In-Reply-To: <cover.1601311803.git.matheus.bernardino@usp.br> When add_delta_base_cache() is called with a base that is already in the cache, no operation is performed. But the check is done after allocating space for a new entry, so we end up leaking memory on the early return. Also, the caller always expect that the base will be inserted, so it never free()'s it. To fix both of these memory leaks, let's move the allocation of a new entry further down in add_delta_base_cache(), and make the function return an integer to indicate whether the insertion was performed or not. Then, make the caller free() the base when needed. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- packfile.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packfile.c b/packfile.c index 0319418d88..177793e01a 100644 --- a/packfile.c +++ b/packfile.c @@ -1471,10 +1471,10 @@ void clear_delta_base_cache(void) } } -static void add_delta_base_cache(struct packed_git *p, off_t base_offset, +static int add_delta_base_cache(struct packed_git *p, off_t base_offset, void *base, unsigned long base_size, enum object_type type) { - struct delta_base_cache_entry *ent = xmalloc(sizeof(*ent)); + struct delta_base_cache_entry *ent; struct list_head *lru, *tmp; /* @@ -1483,7 +1483,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset, * and III might run concurrently across multiple threads). */ if (in_delta_base_cache(p, base_offset)) - return; + return 0; delta_base_cached += base_size; @@ -1495,6 +1495,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset, release_delta_base_cache(f); } + ent = xmalloc(sizeof(*ent)); ent->key.p = p; ent->key.base_offset = base_offset; ent->type = type; @@ -1506,6 +1507,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset, hashmap_init(&delta_base_cache, delta_base_cache_hash_cmp, NULL, 0); hashmap_entry_init(&ent->ent, pack_entry_hash(p, base_offset)); hashmap_add(&delta_base_cache, &ent->ent); + return 1; } int packed_object_info(struct repository *r, struct packed_git *p, @@ -1841,8 +1843,10 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset, * thread could free() it (e.g. to make space for another entry) * before we are done using it. */ - if (!external_base) - add_delta_base_cache(p, base_obj_offset, base, base_size, type); + if (!external_base && !add_delta_base_cache(p, base_obj_offset, + base, base_size, type)) { + free(base); + } free(delta_data); free(external_base); -- 2.28.0
next prev parent reply other threads:[~2020-09-28 16:51 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-25 2:36 RFC - concurrency causes segfault in git grep since 2.26.0 Phil Hord 2020-09-25 5:52 ` Matheus Tavares 2020-09-25 19:53 ` Phil Hord 2020-09-28 16:50 ` [PATCH 0/2] Fix race condition and memory leak in delta base cache Matheus Tavares 2020-09-28 16:50 ` [PATCH 1/2] packfile: fix race condition on unpack_entry() Matheus Tavares 2020-09-28 18:05 ` Junio C Hamano 2020-09-28 16:50 ` Matheus Tavares [this message] 2020-09-28 18:22 ` [PATCH 2/2] packfile: fix memory leak in add_delta_base_cache() Junio C Hamano 2020-09-29 0:01 ` [PATCH v2 0/2] Fix race condition and memory leak in delta base cache Matheus Tavares 2020-09-29 0:01 ` [PATCH v2 1/2] packfile: fix race condition on unpack_entry() Matheus Tavares 2020-10-02 20:06 ` Phil Hord 2020-09-29 0:01 ` [PATCH v2 2/2] packfile: fix memory leak in add_delta_base_cache() Matheus Tavares
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 * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5b6e3019e08c6bccdee29018e99b0c6933fe05e0.1601311803.git.matheus.bernardino@usp.br \ --to=matheus.bernardino@usp.br \ --cc=dstolee@microsoft.com \ --cc=git@vger.kernel.org \ --cc=jonathantanmy@google.com \ --cc=phil.hord@gmail.com \ --cc=stefanbeller@gmail.com \ --subject='Re: [PATCH 2/2] packfile: fix memory leak in add_delta_base_cache()' \ /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
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 NNTP newsgroup(s).