The attached patch attempts to fix the problem reported here:
https://cygwin.com/pipermail/cygwin-developers/2024-December/012725.htmlThe patch is not ready for prime time because I have been unable to test it. When I install the patch, mintty hangs on startup without showing the bash prompt, and a grep process is left running. I can kill the grep process through the Windows Task Manager and then get a bash prompt, but every command I try to run hangs and has to be killed through the Task Manager. In particular, I can't run the test cases that I sent in the above report.
So either there's something wrong with the patch or else it triggers a bug somewhere else. I don't know how to debug this.
Takashi, do you think this could be related to the signal issues that you've been working on? The patch potentially eliminates some SIGBUS signals that were raised incorrectly before, and it also potentially causes some (correct) SIGBUS signals that were not raised before.
Ken
From 3979d0f7a07631c9abac87c4ee935583449e1d42 Mon Sep 17 00:00:00 2001 From: Ken Brown <kbr...@cornell.edu> Date: Wed, 25 Dec 2024 12:35:41 -0500 Subject: [PATCH] Cygwin: mmap: fix mmap_is_attached_or_noreserve After commit 70e476d27be8, mmap_is_attached_or_noreserve would sometimes call VirtualAlloc with MEM_COMMIT on address ranges that were not known to have MEM_RESERVE status. These calls could fail, causing SIGBUS to be raised incorrectly. See https://cygwin.com/pipermail/cygwin-developers/2024-December/012725.html for details. A second problem is that the code would sometimes break out of the main loop without knowing whether attached mappings might still occur later. This could SIGBUS to not be raised when it should be. Fix both of these issues by maintaining the following ordering on the anonymous mmap list: All attached records come first, then those that are noreserve but not attached, then those that are not noreserve. Those that are noreserve but not attached are sorted by address. Add extensive comments explaining all this. Addresses: https://cygwin.com/pipermail/cygwin-developers/2024-December/012725.html Fixes: 70e476d27be8 ("* include/cygwin/version.h: Bump DLL version to 1.7.0") Signed-off-by: Ken Brown <kbr...@cornell.edu> --- winsup/cygwin/mm/mmap.cc | 117 +++++++++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 24 deletions(-) diff --git a/winsup/cygwin/mm/mmap.cc b/winsup/cygwin/mm/mmap.cc index 13418d782baf..10e3f5bdc2b3 100644 --- a/winsup/cygwin/mm/mmap.cc +++ b/winsup/cygwin/mm/mmap.cc @@ -580,18 +580,61 @@ mmap_record::free_fh (fhandler_base *fh) delete fh; } +/* Keep the list of anonymous mappings sorted as follows: All attached + records come first, then those that are noreserve but not attached, + then those that are not noreserve. Those that are noreserve but + not attached are sorted by address. This order of the mappings is + used in mmap_is_attached_or_noreserve. Note: The code that follows + makes implicit use of the fact that attached maps are also + noreserve. */ mmap_record * mmap_list::add_record (mmap_record &r) { - mmap_record *rec = (mmap_record *) ccalloc (HEAP_MMAP, - sizeof (mmap_record) - + MAPSIZE (PAGE_CNT (r.get_len ())) * sizeof (DWORD), 1); - if (!rec) + mmap_record *new_rec = (mmap_record *) ccalloc (HEAP_MMAP, + sizeof (mmap_record) + + MAPSIZE (PAGE_CNT (r.get_len ())) + * sizeof (DWORD), 1); + if (!new_rec) return NULL; - rec->init_page_map (r); + new_rec->init_page_map (r); + + bool new_attached = new_rec->attached (); + bool new_noreserve = new_rec->noreserve (); + caddr_t new_addr = new_rec->get_address (); + + LIST_WRITE_LOCK (); + if (fd != -1 || LIST_EMPTY (&recs)) + { + LIST_INSERT_HEAD (&recs, new_rec, mr_next); + goto out; + } - LIST_INSERT_HEAD (&recs, rec, mr_next); - return rec; + /* Now we're working on the anonymous list, which is non-empty. */ + mmap_record *rec, *prev; + LIST_FOREACH (rec, &recs, mr_next) + { + prev = rec; + if (new_attached) + { + LIST_INSERT_HEAD (&recs, new_rec, mr_next); + goto out; + } + if (new_noreserve && (rec->attached () + || (rec->noreserve () + && new_addr > rec->get_address ()))) + continue; + if (!new_noreserve && rec->noreserve ()) + continue; + LIST_INSERT_BEFORE (rec, new_rec, mr_next); + goto out; + } + + /* If we get here, new_rec should be inserted at the end of the + list, and prev points to the last record. */ + LIST_INSERT_AFTER (prev, new_rec, mr_next); +out: + LIST_WRITE_UNLOCK (); + return new_rec; } void @@ -738,22 +781,24 @@ is_mmapped_region (caddr_t start_addr, caddr_t end_address) /* This function is called from exception_handler when a segmentation violation has occurred. It should also be called from all Cygwin - functions that want to support passing noreserve mmap page addresses - to Windows system calls. In that case, it should be called only after - a system call indicates that the application buffer passed had an - invalid virtual address to avoid any performance impact in non-noreserve - cases. + functions that want to support passing noreserve (anonymous) mmap + page addresses to Windows system calls. In that case, it should be + called only after a system call indicates that the application + buffer passed had an invalid virtual address to avoid any + performance impact in non-noreserve cases. Check if the address range is all within noreserve mmap regions. If so, call VirtualAlloc to commit the pages and return MMAP_NORESERVE_COMMITED - on success. If the page has __PROT_ATTACH (SUSv3 memory protection - extension), or if VirtualAlloc fails, return MMAP_RAISE_SIGBUS. - Otherwise, return MMAP_NONE if the address range is not covered by an - attached or noreserve map. + on success. If some page in the address range has __PROT_ATTACH + (SUSv3 memory protection extension), or if VirtualAlloc fails, + return MMAP_RAISE_SIGBUS. Otherwise, return MMAP_NONE if the + address range is not covered by noreserve maps. On MAP_NORESERVE_COMMITED, the exeception handler should return 0 to allow the application to retry the memory access, or the calling Cygwin - function should retry the Windows system call. */ + function should retry the Windows system call. + + len is always > 0 when this function is called. */ mmap_region_status mmap_is_attached_or_noreserve (void *addr, size_t len) @@ -785,21 +830,45 @@ mmap_is_attached_or_noreserve (void *addr, size_t len) break; } if (!rec->noreserve ()) + /* In view of the ordering of map_list, all remaining maps in + map_list are non-noreserve maps. See add_record for a + description of the ordering. */ break; - size_t commit_len = u_len - (start_addr - u_addr); - if (commit_len > len) - commit_len = len; + /* Now rec is noreserve but not attached, and we have the + following situation: + + |---------|---------------------|-----------------| + start_addr u_addr u_addr+u_len start_addr+len + + The interval [u_addr, u_addr+u_len] is the intersection of + [start_addr, start_addr+len] with the interval of the + mmap_record *rec. Note that u_addr >= start_addr and + u_addr+u_len <= start_addr+len. The picture shows the case + where both inequalities are strict. + + If u_addr > start_addr, then [start_addr, u_addr] lies to the + left of the region of *rec. The ordering of map_list now + implies that all remaining maps in the list are either to the + right of [start_addr, u_addr] or are non-noreserve maps. So + that interval cannot cannot be covered by noreserve maps, and + we will never encounter an attached record later in the list; + we should therefore return MMAP_NONE. + + If u_addr == start_addr, then we try to commit [start_addr, + u_addr+u_len]. */ + + if (u_addr > start_addr) + break; - if (!VirtualAlloc (start_addr, commit_len, MEM_COMMIT, - rec->gen_protect ())) + if (!VirtualAlloc (start_addr, u_len, MEM_COMMIT, rec->gen_protect ())) { ret = MMAP_RAISE_SIGBUS; break; } - start_addr += commit_len; - len -= commit_len; + start_addr += u_len; + len -= u_len; if (!len) { ret = MMAP_NORESERVE_COMMITED; -- 2.45.1
