On Tue, Jun 29, 2021 at 08:11:05AM +0000, Eric Wong wrote:
> 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
