On Jul 20 10:58, Ken Brown via Cygwin-patches wrote: > On 7/20/2020 10:49 AM, Ken Brown via Cygwin-patches wrote: > > On 7/20/2020 10:43 AM, Ken Brown via Cygwin-patches wrote: > > > On 7/20/2020 10:23 AM, Corinna Vinschen wrote: > > > > On Jul 20 09:34, Ken Brown via Cygwin-patches wrote: > > > > > Commit 605bdcd410384dda6db66b9b8cd19e863702e1bb enabled mapping beyond > > > > > EOF in 64 bit environments. But the variable 'orig_len' did not get > > > > > rounded up to a multiple of 64K. This rounding was done on 32 bit > > > > > only. Fix this by rounding up orig_len on 64 bit, in the same place > > > > > where 'len' is rounded up. > > > > > > > > > > One consequence of this bug is that orig_len could be slightly smaller > > > > > than len. Since these are both unsigned values, the statement > > > > > 'orig_len -= len' would then cause orig_len to be huge, and mmap would > > > > > fail with errno EFBIG. > > > > > > > > > > I observed this failure while debugging the problem reported in > > > > > > > > > > https://sourceware.org/pipermail/cygwin/2020-July/245557.html. > > > > > > > > > > The failure can be seen by running the test case in that report under > > > > > gdb or strace. > > > > > --- > > > > > winsup/cygwin/mmap.cc | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc > > > > > index feb9e5d0e..a08d00f83 100644 > > > > > --- a/winsup/cygwin/mmap.cc > > > > > +++ b/winsup/cygwin/mmap.cc > > > > > @@ -1144,6 +1144,7 @@ go_ahead: > > > > > ends in, but there's nothing at all we can do about that. */ > > > > > #ifdef __x86_64__ > > > > > len = roundup2 (len, wincap.allocation_granularity ()); > > > > > + orig_len = roundup2 (orig_len, wincap.allocation_granularity > > > > > ()); > > > > > > > > Wouldn't it be simpler to just check for > > > > > > > > - if (orig_len - len) > > > > + if (orig_len > len) > > > > > > > > in the code following this #if/#else/#endif snippet? > > > > > > I don't think so, because we also want to use the rounded-up value > > > of orig_len further down when we set sigbus_page_len > > > > Actually we first modify orig_len in 'orig_len -= len;' and then use it > > to set sigbus_page_len. In any case, I think it needs to be rounded up > > before being used.
Oh, right. Now I see what you mean. At this point orig_len is still the actual exact size of the file. We only can create the SIGBUS pages starting the next allocation granularity, so, yeah, it makes sense to align orig_size to allocation granularity here. > If you agree, maybe I should modify the commit message to make this point > clear. Might make sense, yeah. While looking into this, I found another bug. The valid_page_len is wrong on 32 bit systems as well. That was supposed to be the remainder of the allocation granularity sized block the file's EOF is part of, but valid_page_len = orig_len % pagesize; is the size of the file's map within that 64K block, not the size of the remainder. That should have been valid_page_len = pagesize - orig_len % pagesize; so this didn't work correctly either. Ultimately, I wonder if we really should keep all the 32 bit OS stuff in. The number of real 32 bit systems (not WOW64) is dwindling fast. Keeping all the AT_ROUND_TO_PAGE stuff in just for what? 2%? of the systems is really not worth it, I guess. Feel free to apply this patch. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer
