| Message ID | 20210629081108.28657-4-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gracefully handling mmap failures | expand |
Eric Wong <e@80x24.org> wrote: > --- a/packfile.c > +++ b/packfile.c > @@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p) > close(fd); > return error("index file %s is too small", path); > } > - idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0); > + do { > + idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE, > + fd, 0); > + } while (idx_map == MAP_FAILED && errno == ENOMEM > + && unuse_one_window(p)); Oops, I dropped extra error handling here :x > close(fd); > > ret = load_idx(path, hashsz, idx_map, idx_size, p);
Eric Wong <e@80x24.org> writes: > Eric Wong <e@80x24.org> wrote: >> --- a/packfile.c >> +++ b/packfile.c >> @@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p) >> close(fd); >> return error("index file %s is too small", path); >> } >> - idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0); >> + do { >> + idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE, >> + fd, 0); >> + } while (idx_map == MAP_FAILED && errno == ENOMEM >> + && unuse_one_window(p)); > > Oops, I dropped extra error handling here :x > >> close(fd); >> >> ret = load_idx(path, hashsz, idx_map, idx_size, p); Something like this, perhaps? You'd also need _() around the error message you added to object-file.c in [2/4], I would think. packfile.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git c/packfile.c w/packfile.c index 2904560f52..b31f14ecb7 100644 --- c/packfile.c +++ w/packfile.c @@ -102,6 +102,10 @@ static int check_packed_git_idx(const char *path, struct packed_git *p) fd, 0); } while (idx_map == MAP_FAILED && errno == ENOMEM && unuse_one_window(p)); + if (idx_map == MAP_FAILED) { + close(fd); + return error_errno(_("%s cannot be mapped"), path); + } close(fd); ret = load_idx(path, hashsz, idx_map, idx_size, p);
Junio C Hamano <gitster@pobox.com> wrote: > Something like this, perhaps? You'd also need _() around the error > message you added to object-file.c in [2/4], I would think. I think I need to drop 1-3 of this series because it seems impossible to account for the mmap-via-malloc cases that still bumps into vm.max_map_count limitations under Linux.
diff --git a/packfile.c b/packfile.c index 4bc7fa0f36..2904560f52 100644 --- a/packfile.c +++ b/packfile.c @@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p) close(fd); return error("index file %s is too small", path); } - idx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0); + do { + idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE, + fd, 0); + } while (idx_map == MAP_FAILED && errno == ENOMEM + && unuse_one_window(p)); close(fd); ret = load_idx(path, hashsz, idx_map, idx_size, p);
This benefits unprivileged users who lack permissions to raise the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource limit. It appears none of our multithreaded code is calling this, or is doing so while holding appropropriate locks to serialize access to the object database. Unconditionally calling unuse_one_window() before xmmap_gently() revealed no test suite failures: --- a/packfile.c +++ b/packfile.c @@ -98,6 +98,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p) return error("index file %s is too small", path); } do { + unuse_one_window(p); idx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0); } while (idx_map == MAP_FAILED && errno == ENOMEM Signed-off-by: Eric Wong <e@80x24.org> --- packfile.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)