diff mbox

Fix wordsize-32 mmap offset for negative value (BZ#18877)

Message ID 55DF7F32.2010308@linaro.org
State Accepted
Commit d3573f61aca67a398de7eaa7593d3973cb5fd154
Headers show

Commit Message

Adhemerval Zanella Aug. 27, 2015, 9:20 p.m. UTC
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

Adhemerval Zanella Aug. 27, 2015, 11:06 p.m. UTC | #1
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.
> 
>
Adhemerval Zanella Aug. 28, 2015, 1:44 p.m. UTC | #2
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.
diff mbox

Patch

diff --git a/posix/Makefile b/posix/Makefile
index 15e8818..39423a9 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -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 \
diff --git a/posix/tst-mmap-offset.c b/posix/tst-mmap-offset.c
new file mode 100644
index 0000000..15dc4da
--- /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.
+
+   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"
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c
index 24835ce..82d8920 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/mmap.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)