Hi Corinna,
On 1/7/2025 3:18 PM, Corinna Vinschen wrote:
On Jan 2 16:42, Ken Brown wrote:
diff --git a/winsup/cygwin/mm/mmap.cc b/winsup/cygwin/mm/mmap.cc
index fc126a87072a..0224779458ef 100644
--- a/winsup/cygwin/mm/mmap.cc
+++ b/winsup/cygwin/mm/mmap.cc
@@ -494,18 +494,24 @@ mmap_record::map_pages (caddr_t addr, SIZE_T len, int
new_prot)
off_t off = addr - get_address ();
off /= wincap.page_size ();
len = PAGE_CNT (len);
- /* First check if the area is unused right now. */
- for (SIZE_T l = 0; l < len; ++l)
- if (MAP_ISSET (off + l))
- {
- set_errno (EINVAL);
- return false;
- }
- if (!noreserve ()
- && !VirtualProtect (get_address () + off * wincap.page_size (),
- len * wincap.page_size (),
- ::gen_protect (new_prot, get_flags ()),
- &old_prot))
+ /* VirtualProtect can only be called on committed pages, so it's not
+ clear how to change protection in the noreserve case. In this
+ case we will therefore require that the pages are unmapped, in
+ order to keep the behavior the same as it was before new_prot was
+ introduced. FIXME: Is there a better way to handle this? */
+ if (noreserve ())
+ {
+ for (SIZE_T l = 0; l < len; ++l)
+ if (MAP_ISSET (off + l))
+ {
+ set_errno (EINVAL);
+ return false;
+ }
+ }
I think this is ok for the time being. But since you're asking...
OK, I'll go ahead and push this, and then...
When we talked about this last month, I already felt that the
implementation is lacking. Actually, it's missing two things which
would improve MAP_NORESERVE mappings considerably:
- mmap_record::prot flag, should be an array of protection bits per page
(POSIX page i e., 64K, not Windows page). Right now we only store the
first protection set at mmap time for the entire map, rather than the
requested protection of every single page. Consider this scenario:
addr = mmap (NULL, 4 * PAGESIZE, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0L);
/* At this point, mmap_record::prot is PROT_READ | PROT_WRITE */
mprotect (addr + 2 * PAGESIZE, PAGESIZE, PROT_READ);
/* At this point, mmap_record::prot *still* is PROT_READ | PROT_WRITE */
/* This write to the memory region will commit page 3... */
addr[2 * PAGE_SIZE + 42] = 1;
/* ...but should have raised a SIGSEGV because the page is supposedly
non-writable! */
The reason is obvious: We only store the start protection PROT_READ |
PROT_WRITE. So if we access the page, mmap_is_attached_or_noreserve()
calls VirtualAlloc() with the start protection bits.
If we store the protection per page, mmap_is_attached_or_noreserve()
can call VirtualAlloc with the correct page protection and we receive
the deserved SIGSEGV.
- For mprotect() it doesn't in fact matter if a page is MAP_NORESERVE or
not. It only matters if the page has been written to (that is, it has
been committed) or not (that is, it's still reserved).
If the page is still only reserved map_pages() can just change the
protection bits in mmap_record::prot[page]. If the page was already
commited, it can additionally call VirtualProtect() with the new
per-page protection.
We don't even need to keep track of reserved vs. commited, because
VirtualQuery() already does that for us.
I'll try to implement this idea. Thanks for the suggestion.
Ken
