| Message ID | 20210629081108.28657-3-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:06AM +0000, Eric Wong wrote: > This benefits unprivileged users who lack permissions to raise > the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource > limit. > > Multi-threaded callers to map_loose_object_1 (e.g. "git grep") > appear to guard against thread-safety problems. Other > multi-core aware pieces of code (e.g. parallel-checkout) uses > child processes This one makes me slightly more nervous than the last, because we don't know if somebody higher up the call stack might be expecting to use that window again. I _think_ this one is OK, because we would never read a loose object in the process of reading a packed one. I thought we might in some rare cases with corruption (e.g., B is a delta against A; we fail to read A because it's corrupt, so we look elsewhere for a copy). I don't think the current code is quite that clever, though. If B were corrupted, we'd look elsewhere for a duplicate, but that logic doesn't apply in the middle of a delta resolution (even though there are corruption cases it would help recover from). So I don't think this is introducing any bugs. But it worries me a bit that it creates an opportunity for a subtle and hard-to-trigger situation if we do change seemingly-unrelated code. But looking more carefully at unuse_one_window(), I think the worst case is a slight performance drop, as we unmap the window and then re-map when somebody higher up the call-stack needs it again. My real worry was that we could trigger a case where somebody was holding onto a pointer to the bytes. But I think use_pack() will mark those bytes via inuse_cnt, and then unuse_one_window() will never drop a window that has a non-zero inuse_count. So in that sense all of your patches should be correct without thinking too hard no them. Sure, a performance drop from having to re-map the window later isn't great, but if the alternative in each case is calling die(), it's a strict improvement. -Peff
On Tue, Jun 29 2021, Eric Wong wrote: > This benefits unprivileged users who lack permissions to raise > the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource > limit. > > Multi-threaded callers to map_loose_object_1 (e.g. "git grep") > appear to guard against thread-safety problems. Other > multi-core aware pieces of code (e.g. parallel-checkout) uses > child processes s/uses/use/ & missing full-stop.
diff --git a/object-file.c b/object-file.c index f233b440b2..4c043f1f3c 100644 --- a/object-file.c +++ b/object-file.c @@ -1196,7 +1196,13 @@ static void *map_loose_object_1(struct repository *r, const char *path, close(fd); return NULL; } - map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0); + do { + map = xmmap_gently(NULL, *size, PROT_READ, + MAP_PRIVATE, fd, 0); + } while (map == MAP_FAILED && errno == ENOMEM + && unuse_one_window(NULL)); + if (map == MAP_FAILED) + die_errno("%s cannot be mapped", path); } close(fd); } diff --git a/packfile.c b/packfile.c index a0da790fb4..4bc7fa0f36 100644 --- a/packfile.c +++ b/packfile.c @@ -265,7 +265,7 @@ static void scan_windows(struct packed_git *p, } } -static int unuse_one_window(struct packed_git *current) +int unuse_one_window(struct packed_git *current) { struct packed_git *p, *lru_p = NULL; struct pack_window *lru_w = NULL, *lru_l = NULL; diff --git a/packfile.h b/packfile.h index 3ae117a8ae..da9a061e30 100644 --- a/packfile.h +++ b/packfile.h @@ -196,4 +196,6 @@ int is_promisor_object(const struct object_id *oid); int load_idx(const char *path, const unsigned int hashsz, void *idx_map, size_t idx_size, struct packed_git *p); +int unuse_one_window(struct packed_git *); + #endif
This benefits unprivileged users who lack permissions to raise the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource limit. Multi-threaded callers to map_loose_object_1 (e.g. "git grep") appear to guard against thread-safety problems. Other multi-core aware pieces of code (e.g. parallel-checkout) uses child processes Forcing a call to unuse_one_window() once before xmmap_gently() revealed no test suite failures: --- a/object-file.c +++ b/object-file.c @@ -1197,6 +1197,7 @@ static void *map_loose_object_1(struct repository *r, const char *path, return NULL; } do { + unuse_one_window(NULL); map = xmmap_gently(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0); } while (map == MAP_FAILED && errno == ENOMEM Signed-off-by: Eric Wong <e@80x24.org> --- object-file.c | 8 +++++++- packfile.c | 2 +- packfile.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)