Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
On 2020-07-20 15:08, Ken Brown via Cygwin-patches wrote: > On 7/20/2020 4:29 PM, Brian Inglis wrote: >> On 2020-07-20 09:41, Corinna Vinschen wrote: >>> 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. > [...] >> If you don't want to ask and perhaps reconsider the impact of your approach, >> maybe give folks a heads up on the mailing list that the current release >> will be >> the last to support 32 bit Windows? > > Corinna didn't propose dropping support for 32 bit Windows. She proposed > dropping a feature that works *only* on 32 bit Windows. Sorry if I misunderstood, but it sounded more drastic than that, and required for the code to work compatibly on Windows 32 bit, rather than supporting a different feature that only worked on Windows 32 bit, as anything non-Linux/BSD/POSIX can be considered non-essential if support is not widespread, nor essential for critical packages. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in IEC units and prefixes, physical quantities in SI.]
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
On 7/20/2020 4:29 PM, Brian Inglis wrote: On 2020-07-20 09:41, Corinna Vinschen wrote: 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. [...] If you don't want to ask and perhaps reconsider the impact of your approach, maybe give folks a heads up on the mailing list that the current release will be the last to support 32 bit Windows? Corinna didn't propose dropping support for 32 bit Windows. She proposed dropping a feature that works *only* on 32 bit Windows. Ken
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
On 2020-07-20 09:41, Corinna Vinschen wrote: > 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. A lot of older/smaller laptops, especially low cost budget models for lower income groups including students, seniors and for developing countries, Windows (Surface, etc.) tablets, and VMs supporting legacy systems and applications, run Windows 32 bit, and MS still sells W10 and supports 32 bit for that reason, except in OEM channels going forward from the 2020-04 release[1]. About 40% of W7 systems sold were 32 bit, with about 25% of systems still running W7, so 10% of systems could still be 32-bit. If you don't want to ask and perhaps reconsider the impact of your approach, maybe give folks a heads up on the mailing list that the current release will be the last to support 32 bit Windows? -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised. [Data in IEC units and prefixes, physical quantities in SI.] [1] 2004 looks like it is 16 years old - delete software still using 2 digit years for anything, as we will have many ambiguities until 2032!
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
On 7/20/2020 11:41 AM, Corinna Vinschen wrote: 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. I agree. It complicates the code with very little benefit. Ken
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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. If you agree, maybe I should modify the commit message to make this point clear. Ken
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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. Ken
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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. Ken
Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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? Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer
[PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit
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 ()); #else len = roundup2 (len, wincap.is_wow64 () ? wincap.allocation_granularity () : wincap.page_size ()); -- 2.27.0
