mmap_is_attached_or_noreserve

[Ken Brown]

On 12/25/2024 6:06 PM, Ken Brown wrote:
> On 12/25/2024 1:08 PM, Ken Brown wrote:
>> Hi Corinna,
>>
>> I know you're on vacation, but I'm writing this now while it's fresh 
>> in my mind.  We can discuss it when you return.
>>
>> This is a follow-up to
>>
>>    https://cygwin.com/pipermail/cygwin-developers/2024- 
>> December/012715.html
>>
>> but I'm starting a new thread for clarity, and I'll start from scratch.
>>
>> I'm back to thinking that mmap_is_attached_or_noreserve is broken.  
>> I'm attaching two test cases to show this.  To run them, create a file 
>> hello.txt with one line, and then compile and run the two programs.
>>
>> They both take a region of size 128K and map the two halves of it 
>> separately with MAP_NORESERVE.  The only difference between the two 
>> cases is the order in which the halves are mapped.  They then call 
>> read(), passing the whole region of size 128K as buffer.  If 
>> mmap_is_attached_or_noreserve is working, the calls to read should 
>> succeed.  But only the one in mmap_noreserve2 succeeds, and the other 
>> fails with a bus error.  I'll now try to explain what's going on.
>>
>> The task is to find one or more noreserve records that completely 
>> cover the interval [addr, addr+len].
>>
>> After a successful call to match(start_addr, len, u_addr, u_len), 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.  
>> Note that u_addr  >= start_addr and u_addr+u_len <= start_addr+len, 
>> but I've shown in the picture the case where both inequalities are 
>> strict.
>>
>> At this point we know that the intersection is contained in a 
>> noreserve mmap_record.  But we commit the entire interval [start_addr, 
>> u_addr+u_len], whose length is commit_len, which is written as
>>
>>    u_len - (start_addr - u_addr) = (u_addr + u_len) - start_addr.
>>
>> There's no problem if u_addr = start_addr.  But if u_addr > 
>> start_addr, then the commit includes [start_addr, u_addr], which is 
>> not at this point known to be contained in a noreserve mmap_record.  
>> Moreover, even if it is contained in a noreserve record that we just 
>> haven't come to yet, experiments show that the call to VirtualAlloc in 
>> line 794 fails in this case.  Apparently you can't commit in one call 
>> to VirtualAlloc a region that was reserved [Windows terminology] in 
>> two separate calls.  I can't find this documented anywhere, but that's 
>> what happens in the attached mmap_noreserve1.c, as I verified by 
>> stepping through the code under gdb.
>>
>> As I said in an earlier message, I don't see how to fix this except by 
>> making sure that the noreserve maps in map_list occur in order of 
>> their starting address.  Then if we ever find u_addr > start_addr, we 
>> know we'll never find a noreserve mmap_record covering [start_addr, 
>> u_addr], and we can break out of the loop.  If you agree that there's 
>> a problem, does this seem like the right fix?
>>
>> I apologize for the length of this message, but I wanted to write 
>> everything out in detail in order to be clear.  I've written a patch 
>> (also attached) that attempts to do does what I suggested in the 
>> previous paragraph.  Unfortunately, when I install it, mintty hangs on 
>> startup without showing the bash prompt, and a grep process is left 
>> running.  So there must be something wrong with my patch, or else it 
>> triggers a latent bug somewhere else.
>>
>> Takashi, if you're reading this, could you take a look at my patch and 
>> see if you spot any obvious errors?  Or do you think this could be 
>> related to your recent changes?
> 
> I've find two errors, one in my patch and one in the original code: 
> There were two places in the main loop of mmap_is_attached_or_noreserve 
> where there was a "break" that should have been "continue".  Version 2 
> of the patch (attached) tries to fix these.  But I still get the same 
> mintty hang on startup.  So there's still something wrong.
> 
> Note that the original bug that I'm trying to fix caused SIGBUS to be 
> raised in some places where it shouldn't have been (due to a failed call 
> to VirtualAlloc).  The new break/continue bug I just found had the 
> opposite effect: it caused SIGBUS not to be raised in some places where 
> it should have been.
I've cleaned up and improved the patch, and I've sent it to 
cygwin-patches.  I'm stuck at this point.

Ken