Fix wordsize-32 mmap offset for negative value (BZ#18877)
Commit Message
On 27-08-2015 17:51, Dmitry V. Levin wrote:
> On Thu, Aug 27, 2015 at 05:13:13PM -0300, Adhemerval Zanella wrote:
>> This patch fixes the default wordsize-32 mmap implementation offset
>> calculation for negative values. Current code uses signed shift
>> operation to calculate the multiple size to use with syscall and
>> it is implementation defined. Change it to use a division base
>> on mmap page size (default being as before, 4096).
>>
>> Tested on armv7hf.
>>
>> [BZ #18877]
>> * misc/Makefile (tests): Add tst-mmap
>> * sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
>> offset calculation for negative values.
>> * misc/tst-mmap.c: New file.
>
> As we have posix/tst-mmap.c already, my idea was to call it
> tst-mmap-offset.c,
>
>> + char fname[] = "tst-mmap-offset-XXXXXX";
>
> hence the name of this temporary file.
>
>> - offset >> MMAP_PAGE_SHIFT);
>> + offset/MMAP_PAGE_UNIT);
>
> offset / MMAP_PAGE_UNIT
Thanks for the review, what about now:
--
[BZ #18877]
* posix/Makefile (tests): Add tst-mmap
* sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
offset calculation for negative values.
* posix/tst-mmap-offset.c: New file.
--
Comments
On Thu, Aug 27, 2015 at 06:20:50PM -0300, Adhemerval Zanella wrote:
[...]
> Thanks for the review, what about now:
>
> --
>
> [BZ #18877]
> * posix/Makefile (tests): Add tst-mmap
It's called tst-mmap-offset now.
Do not forget about trailing dot.
> * sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
> offset calculation for negative values.
> * posix/tst-mmap-offset.c: New file.
Let's keep the list of files in some order.
> --- /dev/null
> +++ b/posix/tst-mmap-offset.c
> @@ -0,0 +1,67 @@
> +/* BZ #18877 mmap offset test.
> +
> + Copyright (C) 2015 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
Contributed by me, 2015. :)
> +/* Check if negative offset are handled correctly by mmap. */;
offsetS are.
The trailing semicolon is not needed.
On 27-08-2015 18:48, Dmitry V. Levin wrote:
> On Thu, Aug 27, 2015 at 06:20:50PM -0300, Adhemerval Zanella wrote:
> [...]
>> Thanks for the review, what about now:
>>
>> --
>>
>> [BZ #18877]
>> * posix/Makefile (tests): Add tst-mmap
>
> It's called tst-mmap-offset now.
> Do not forget about trailing dot.
>
>> * sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
>> offset calculation for negative values.
>> * posix/tst-mmap-offset.c: New file.
>
> Let's keep the list of files in some order.
>
>> --- /dev/null
>> +++ b/posix/tst-mmap-offset.c
>> @@ -0,0 +1,67 @@
>> +/* BZ #18877 mmap offset test.
>> +
>> + Copyright (C) 2015 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>
> Contributed by me, 2015. :)
We do not use 'Contributed by' anymore, we use it the ChangeLog entry
(which I will add you as well).
>
>> +/* Check if negative offset are handled correctly by mmap. */;
>
> offsetS are.
> The trailing semicolon is not needed.
>
>
On Thu, Aug 27, 2015 at 08:06:55PM -0300, Adhemerval Zanella wrote:
> On 27-08-2015 18:48, Dmitry V. Levin wrote:
> > On Thu, Aug 27, 2015 at 06:20:50PM -0300, Adhemerval Zanella wrote:
> > [...]
> >> Thanks for the review, what about now:
> >>
> >> --
> >>
> >> [BZ #18877]
> >> * posix/Makefile (tests): Add tst-mmap
> >
> > It's called tst-mmap-offset now.
> > Do not forget about trailing dot.
> >
> >> * sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
> >> offset calculation for negative values.
> >> * posix/tst-mmap-offset.c: New file.
> >
> > Let's keep the list of files in some order.
> >
> >> --- /dev/null
> >> +++ b/posix/tst-mmap-offset.c
> >> @@ -0,0 +1,67 @@
> >> +/* BZ #18877 mmap offset test.
> >> +
> >> + Copyright (C) 2015 Free Software Foundation, Inc.
> >> + This file is part of the GNU C Library.
> >
> > Contributed by me, 2015. :)
>
> We do not use 'Contributed by' anymore, we use it the ChangeLog entry
> (which I will add you as well).
Indeed.
> >> +/* Check if negative offset are handled correctly by mmap. */;
> >
> > offsetS are.
> > The trailing semicolon is not needed.
LGTM, assuming these typos are corrected.
On 28-08-2015 04:05, Dmitry V. Levin wrote:
> On Thu, Aug 27, 2015 at 08:06:55PM -0300, Adhemerval Zanella wrote:
>> On 27-08-2015 18:48, Dmitry V. Levin wrote:
>>> On Thu, Aug 27, 2015 at 06:20:50PM -0300, Adhemerval Zanella wrote:
>>> [...]
>>>> Thanks for the review, what about now:
>>>>
>>>> --
>>>>
>>>> [BZ #18877]
>>>> * posix/Makefile (tests): Add tst-mmap
>>>
>>> It's called tst-mmap-offset now.
>>> Do not forget about trailing dot.
>>>
>>>> * sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c (__mmap): Fix
>>>> offset calculation for negative values.
>>>> * posix/tst-mmap-offset.c: New file.
>>>
>>> Let's keep the list of files in some order.
>>>
>>>> --- /dev/null
>>>> +++ b/posix/tst-mmap-offset.c
>>>> @@ -0,0 +1,67 @@
>>>> +/* BZ #18877 mmap offset test.
>>>> +
>>>> + Copyright (C) 2015 Free Software Foundation, Inc.
>>>> + This file is part of the GNU C Library.
>>>
>>> Contributed by me, 2015. :)
>>
>> We do not use 'Contributed by' anymore, we use it the ChangeLog entry
>> (which I will add you as well).
>
> Indeed.
>
>>>> +/* Check if negative offset are handled correctly by mmap. */;
>>>
>>> offsetS are.
>>> The trailing semicolon is not needed.
>
> LGTM, assuming these typos are corrected.
>
Pushed upstream as d3573f61aca67a398de7eaa7593d3973cb5fd154.
@@ -64,7 +64,7 @@ routines := \
aux := init-posix environ
tests := tstgetopt testfnm runtests runptests \
tst-preadwrite tst-preadwrite64 test-vfork regexbug1 \
- tst-mmap tst-getaddrinfo tst-truncate \
+ tst-mmap tst-mmap-offset tst-getaddrinfo tst-truncate \
tst-truncate64 tst-fork tst-fnmatch tst-regexloc tst-dir \
tst-chmod bug-regex1 bug-regex2 bug-regex3 bug-regex4 \
tst-gnuglob tst-regex bug-regex5 bug-regex6 bug-regex7 \
new file mode 100644
@@ -0,0 +1,67 @@
+/* BZ #18877 mmap offset test.
+
+ Copyright (C) 2015 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mman.h>
+
+static int
+printmsg (int rc, const char *msg)
+{
+ printf ("%s failed: %m\n", msg);
+ return rc;
+}
+
+/* Check if negative offset are handled correctly by mmap. */;
+static int
+do_test (void)
+{
+ const int prot = PROT_READ | PROT_WRITE;
+ const int flags = MAP_SHARED;
+ const unsigned long length = 0x10000;
+ const unsigned long offset = 0xace00000;
+ const unsigned long size = offset + length;
+ void *addr;
+ int fd;
+ char fname[] = "tst-mmap-offset-XXXXXX";
+
+ fd = mkstemp64 (fname);
+ if (fd < 0)
+ return printmsg (1, "mkstemp");
+
+ if (unlink (fname))
+ return printmsg (1, "unlink");
+
+ if (ftruncate64 (fd, size))
+ return printmsg (0, "ftruncate64");
+
+ addr = mmap (NULL, length, prot, flags, fd, offset);
+ if (MAP_FAILED == addr)
+ return printmsg (1, "mmap");
+
+ /* This memcpy is likely to SIGBUS if mmap has messed up with offset. */
+ memcpy (addr, fname, sizeof (fname));
+
+ return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
@@ -21,20 +21,20 @@
#include <errno.h>
#include <sysdep.h>
-#ifndef MMAP_PAGE_SHIFT
-#define MMAP_PAGE_SHIFT 12
+#ifndef MMAP_PAGE_UNIT
+#define MMAP_PAGE_UNIT 4096UL
#endif
__ptr_t
__mmap (__ptr_t addr, size_t len, int prot, int flags, int fd, off_t offset)
{
- if (offset & ((1 << MMAP_PAGE_SHIFT) - 1))
+ if (offset & (MMAP_PAGE_UNIT - 1))
{
__set_errno (EINVAL);
return MAP_FAILED;
}
return (__ptr_t) INLINE_SYSCALL (mmap2, 6, addr, len, prot, flags, fd,
- offset >> MMAP_PAGE_SHIFT);
+ offset / MMAP_PAGE_UNIT);
}
weak_alias (__mmap, mmap)