git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, phil.hord@gmail.com, dstolee@microsoft.com,
	jonathantanmy@google.com, stefanbeller@gmail.com
Subject: Re: [PATCH 1/2] packfile: fix race condition on unpack_entry()
Date: Mon, 28 Sep 2020 11:05:52 -0700	[thread overview]
Message-ID: <xmqqpn65n5dr.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <42a7948f94cb57ebd9c37c3850b46b1ac9813ec6.1601311803.git.matheus.bernardino@usp.br> (Matheus Tavares's message of "Mon, 28 Sep 2020 13:50:34 -0300")

Matheus Tavares <matheus.bernardino@usp.br> writes:

> To fix the race condition (and later segmentation fault), let's reorder
> the aforementioned steps so that the lock is not released between 1.
> and 3.

In other words, we hold the base in core only for ourselves without
adding it to the base cache, apply the delta to produce the result
and then place the base in the cache, and the reason why this change
fixes the breakage is because the base we have locally and not in
cache will not be seen by other people and will not be freed without
our consent?  Which does make sense.

But I was confused by the explanation "lock is not released".  We do
release the same lock when we call unpack_compressed_entry(), and
reaquire it before the unpack_compressed_entry() returns.  What the
reordering achieves is to protect the base from getting evicted when
the unlocking and relocing happens, no?

Thanks.

  reply	other threads:[~2020-09-28 18:06 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 [this message]
2020-09-28 16:50       ` [PATCH 2/2] packfile: fix memory leak in add_delta_base_cache() Matheus Tavares
2020-09-28 18:22         ` 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=xmqqpn65n5dr.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=matheus.bernardino@usp.br \
    --cc=phil.hord@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH 1/2] packfile: fix race condition on unpack_entry()' \
    /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).