| Message ID | 20210629081108.28657-2-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gracefully handling mmap failures | expand |
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
diff --git a/packfile.c b/packfile.c index 755aa7aec5..a0da790fb4 100644 --- a/packfile.c +++ b/packfile.c @@ -648,9 +648,12 @@ unsigned char *use_pack(struct packed_git *p, while (packed_git_limit < pack_mapped && unuse_one_window(p)) ; /* nothing */ - win->base = xmmap_gently(NULL, win->len, - PROT_READ, MAP_PRIVATE, - p->pack_fd, win->offset); + do { + win->base = xmmap_gently(NULL, win->len, + PROT_READ, MAP_PRIVATE, + p->pack_fd, win->offset); + } while (win->base == MAP_FAILED && errno == ENOMEM + && unuse_one_window(p)); if (win->base == MAP_FAILED) die_errno("packfile %s cannot be mapped", p->pack_name);
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); Signed-off-by: Eric Wong <e@80x24.org> --- packfile.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)