Re: [PATCH 1/4] use_pack: attempt to handle ENOMEM from mmap
- From
Jeff King <peff@peff.net>
- Date
- Jun 30, 2021, 02:30 UTC
- Message-ID
- <YNvXTf6PG2zhbjdh@coredump.intra.peff.net>
- In-Reply-To
- <20210629081108.28657-2-e@80x24.org>
On Tue, Jun 29, 2021 at 08:11:05AM +0000, Eric Wong wrote:
Show 22 quoted lines
> Since use_pack() can already safely munmap packs to respect
> core.packedGitLimit, attempt to gracefully handle ENOMEM
> errors the same way by unmapping a window and retrying.
>
> This benefits unprivileged users who lack permissions to raise
> the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
> limit.
>
> I've also verified it is safe to release a pack here by
> unconditionally calling unuse_one_window() before
> xmmap_gently():
>
> --- a/packfile.c
> +++ b/packfile.c
> @@ -649,6 +649,7 @@ unsigned char *use_pack(struct packed_git *p,
> && unuse_one_window(p))
> ; /* nothing */
> do {
> + unuse_one_window(p);
> win->base = xmmap_gently(NULL, win->len,
> PROT_READ, MAP_PRIVATE,
> p->pack_fd, win->offset);I don't find that test-diff all that compelling, because we don't know which window will get unused. I.e., if there is one that will get racily unused, we might not hit it. I think it would be a lot more interesting for finding problems if it did:
while (unuse_one_window(p)) ;
to clear them all.
That said, I think this must be obviously correct because the code above will potentially have just called unuse_one_window(p) already. So at least if not obviously correct, no more buggy than the previous code. :)
-Peff