Opened 12 months ago
Closed 11 months ago
#18733 closed bug (fixed)
mmap fails with out of memory if map big read-only file with MAP_PRIVATE flag
Reported by: | X512 | Owned by: | nobody |
---|---|---|---|
Priority: | normal | Milestone: | R1/beta5 |
Component: | System/Kernel | Version: | R1/Development |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Platform: | All |
Description
This is hrev57493.
map_file
syscall used in mmap
implementation fails to map read-only file on disk if mapped size is larger than total amount of RAM. It cause Git to fail to fetch big repositories:
> git fetch origin-new remote: Enumerating objects: 433106, done. remote: Counting objects: 100% (175641/175641), done. remote: Total 433106 (delta 175641), reused 175641 (delta 175641), pack-reused 257465 Receiving objects: 100% (433106/433106), 992.65 MiB | 3.78 MiB/s, done. fatal: packfile .git/objects/pack/pack-ab36c9a68f870060114341fac8c873d549467667.pack cannot be mapped: Out of memory fatal: fetch-pack: invalid index-pack output
Changing MAP_PRIVATE
flag to MAP_SHARED
fixes problem and Git fetch succeeds.
As I understand there are no difference between MAP_PRIVATE
and MAP_SHARED
if file is mapped without write access.
Quick patch that fixes issue:
-
src/system/libroot/posix/sys/mman.cpp
diff --git a/src/system/libroot/posix/sys/mman.cpp b/src/system/libroot/posix/sys/mman.cpp index f91771a7ef..2d7ad640d5 100644
a b mmap(void* address, size_t length, int protection, int flags, int fd, 125 125 } 126 126 127 127 // translate mapping, address specification, and protection 128 int mapping = (flags & MAP_SHARED) != 0 128 int mapping = (flags & MAP_SHARED) != 0 || (protection & PROT_WRITE) == 0 129 129 ? REGION_NO_PRIVATE_MAP : REGION_PRIVATE_MAP; 130 130 131 131 uint32 addressSpec;
Attachments (1)
Change History (6)
comment:1 by , 12 months ago
comment:2 by , 11 months ago
As I understand there are no difference between MAP_PRIVATE and MAP_SHARED if file is mapped without write access.
This is true, however, files mapped without MAP_SHARED (i.e. with REGION_PRIVATE_MAP) can have PROT_WRITE added later on (via mprotect), after mmap was called, and all will work as expected. If we were to optimize this inside mmap or in the kernel, then that would prevent applications from doing that.
So, I don't think we can make this change either in libroot or in the kernel directly.
Instead, what we could do is to avoid committing memory in this case, and then only commit it when mprotect() was called (and thus fail in mprotect if memory can't be reserved.)
It looks like set_area_protection already alters commitment on changing from non-writable -> writable, but mprotect doesn't.
comment:3 by , 11 months ago
files mapped without MAP_SHARED (i.e. with REGION_PRIVATE_MAP) can have PROT_WRITE added later on (via mprotect)
I suspect mprotect
should fail with out of memory in this case.
So, I don't think we can make this change either in libroot
Yes, I am aware that my quick patch is not a proper solution. I need this patch to be able to pull large Git repositories such as Webkit.
by , 11 months ago
Attachment: | mmap_private_mprotect.cpp added |
---|
comment:4 by , 11 months ago
Test case attached that reproduces this problem (requires creating a file larger than system RAM for testing.)
Proposed fix: https://review.haiku-os.org/c/haiku/+/7339
comment:5 by , 11 months ago
Milestone: | Unscheduled → R1/beta5 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Fixed in hrev57517.
I think this optimization needs to be done in the kernel, because I think it's checked whether the FD was opened read-write or read-only? If the FD was opened read-only, then yes, we can use MAP_SHARED, I think. But I'd have to check.