git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 1/2] packfile: fix race condition on unpack_entry()

From
Phil Hord <phil.hord@gmail.com>
Date
Oct 2, 2020, 20:06 UTC
Message-ID
<CABURp0ovz0G-mYDv+CmL_pSu09WDJTxNQodpnyT3MrYjKjusEw@mail.gmail.com>
In-Reply-To
<948d07673f3b7eebf3d776ec2c785e65228ed185.1601337543.git.matheus.bernardino@usp.br>

On Mon, Sep 28, 2020 at 5:02 PM Matheus Tavares <matheus.bernardino@usp.br> wrote:

Show 32 quoted lines
>
> The third phase of unpack_entry() performs the following sequence in a
> loop, until all the deltas enumerated in phase one are applied and the
> entry is fully reconstructed:
>
> 1. Add the current base entry to the delta base cache
> 2. Unpack the next delta
> 3. Patch the unpacked delta on top of the base
>
> When the optional object reading lock is enabled, the above steps will
> be performed while holding the lock. However, step 2. momentarily
> releases it so that inflation can be performed in parallel for increased
> performance. Because the `base` buffer inserted in the cache at 1. is
> not duplicated, another thread can potentially free() it while the lock
> is released at 2. (e.g. when there is no space left in the cache to
> insert another entry). In this case, the later attempt to dereference
> `base` at 3. will cause a segmentation fault. This problem was observed
> during a multithreaded git-grep execution on a repository with large
> objects.
>
> To fix the race condition (and later segmentation fault), let's reorder
> the aforementioned steps so that `base` is only added to the cache at
> the end. This will prevent the buffer from being released by another
> thread while it is still in use. An alternative solution which would not
> require the reordering would be to duplicate `base` before inserting it
> in the cache. However, as Phil Hord mentioned, memcpy()'ing large bases
> can negatively affect performance: in his experiments, this alternative
> approach slowed git-grep down by 10% to 20%.
>
> Reported-by: Phil Hord <phil.hord@gmail.com>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---

Thanks for looking after this so quickly. This all looks good to me, and I confirmed it does fix the problems I was seeing.

Phil
Previous: Matheus TavaresNext: Matheus Tavares
Message 11 of 12 in “RFC - concurrency causes segfault in git grep since 2.26.0”
  1. Phil HordSep 25, 2020
  2. Matheus TavaresSep 25, 2020
  3. Phil HordSep 25, 2020
  4. 0/2 Fix race condition and memory leak in delta base cacheMatheus Tavares, Sep 28, 2020
  5. 1/2 packfile: fix race condition on unpack_entry()Matheus Tavares, Sep 28, 2020
  6. Junio C HamanoSep 28, 2020
  7. 2/2 packfile: fix memory leak in add_delta_base_cache()Matheus Tavares, Sep 28, 2020
  8. Junio C HamanoSep 28, 2020
  9. 0/2 Fix race condition and memory leak in delta base cacheMatheus Tavares, Sep 29, 2020
  10. 1/2 packfile: fix race condition on unpack_entry()Matheus Tavares, Sep 29, 2020
  11. Phil HordOct 2, 2020
  12. 2/2 packfile: fix memory leak in add_delta_base_cache()Matheus Tavares, Sep 29, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.