Remove incorrect alloc_size attribute from pvalloc [BZ #25401]
diff mbox series

Message ID 87h80uqsv4.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • Remove incorrect alloc_size attribute from pvalloc [BZ #25401]
Related show

Commit Message

Florian Weimer Jan. 17, 2020, 10:28 a.m. UTC
pvalloc is guarantueed to round up the allocation size to the page
size, so applications can assume that the memory region is larger
than the passed-in argument.  The alloc_size attribute cannot express
that.

The test case is based on a suggestion from Jakub Jelinek.

This fixes commit 9bf8e29ca136094f73f69f725f15c51facc97206 ("malloc:
make malloc fail with requests larger than PTRDIFF_MAX (BZ#23741)").

-----
 malloc/Makefile              |  2 +-
 malloc/malloc.h              |  3 +--
 malloc/tst-pvalloc-fortify.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

Comments

Adhemerval Zanella Jan. 17, 2020, 12:14 p.m. UTC | #1
On 17/01/2020 07:28, Florian Weimer wrote:
> pvalloc is guarantueed to round up the allocation size to the page
> size, so applications can assume that the memory region is larger
> than the passed-in argument.  The alloc_size attribute cannot express
> that.
> 
> The test case is based on a suggestion from Jakub Jelinek.
> 
> This fixes commit 9bf8e29ca136094f73f69f725f15c51facc97206 ("malloc:
> make malloc fail with requests larger than PTRDIFF_MAX (BZ#23741)").

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> -----
>  malloc/Makefile              |  2 +-
>  malloc/malloc.h              |  3 +--
>  malloc/tst-pvalloc-fortify.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 734efe368d..984045b5b9 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -27,7 +27,7 @@ headers := $(dist-headers) obstack.h mcheck.h
>  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>  	 tst-mcheck tst-mallocfork tst-trim1 \
>  	 tst-malloc-usable tst-realloc tst-reallocarray tst-posix_memalign \
> -	 tst-pvalloc tst-memalign tst-mallopt \
> +	 tst-pvalloc tst-pvalloc-fortify tst-memalign tst-mallopt \
>  	 tst-malloc-backtrace tst-malloc-thread-exit \
>  	 tst-malloc-thread-fail tst-malloc-fork-deadlock \
>  	 tst-mallocfork2 \

Ok.

> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 0c76264421..a6903fdd54 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -71,8 +71,7 @@ extern void *valloc (size_t __size) __THROW __attribute_malloc__
>  
>  /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up
>     __size to nearest pagesize. */
> -extern void *pvalloc (size_t __size) __THROW __attribute_malloc__
> -     __attribute_alloc_size__ ((1)) __wur;
> +extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur;
>  
>  /* Underlying allocation function; successive calls should return
>     contiguous pieces of memory.  */

Ok.

> diff --git a/malloc/tst-pvalloc-fortify.c b/malloc/tst-pvalloc-fortify.c
> new file mode 100644
> index 0000000000..391b7fa2f5
> --- /dev/null
> +++ b/malloc/tst-pvalloc-fortify.c
> @@ -0,0 +1,48 @@
> +/* Test fortify-source allocation size handling in pvalloc (bug 25401).
> +   Copyright (C) 2020 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; see the file COPYING.LIB.  If
> +   not, see <https://www.gnu.org/licenses/>.  */
> +
> +#undef _FORTIFY_SOURCE
> +#define _FORTIFY_SOURCE 2
> +#include <malloc.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* The test below assumes that pvalloc rounds up the allocation size
> +     to at least 8.  */
> +  TEST_VERIFY (xsysconf (_SC_PAGESIZE) >= 8);
> +
> +  void *p = pvalloc (5);
> +  TEST_VERIFY_EXIT (p != NULL);
> +
> +  /* This is valid assuming the page size is at least 8 because
> +     pvalloc rounds up the allocation size to a multiple of the page
> +     size.  Due to bug 25041, this used to trigger a compiler
> +     warning.  */
> +  strcpy (p, "abcdefg");
> +
> +  asm ("" : : "g" (p) : "memory"); /* Optimization barrier.  */
> +  TEST_VERIFY (malloc_usable_size (p) >= xsysconf (_SC_PAGESIZE));
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Ok.
Florian Weimer Jan. 17, 2020, 1:14 p.m. UTC | #2
* Adhemerval Zanella:

> On 17/01/2020 07:28, Florian Weimer wrote:
>> pvalloc is guarantueed to round up the allocation size to the page
>> size, so applications can assume that the memory region is larger
>> than the passed-in argument.  The alloc_size attribute cannot express
>> that.
>> 
>> The test case is based on a suggestion from Jakub Jelinek.
>> 
>> This fixes commit 9bf8e29ca136094f73f69f725f15c51facc97206 ("malloc:
>> make malloc fail with requests larger than PTRDIFF_MAX (BZ#23741)").
>
> LGTM, thanks.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Thanks.

Siddhesh, should we put this into the release?

Florian
Siddhesh Poyarekar Jan. 17, 2020, 2:04 p.m. UTC | #3
On 17/01/20 6:44 pm, Florian Weimer wrote:
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> Thanks.
> 
> Siddhesh, should we put this into the release?
> 

Yes please.  Well contained bug fixes are generally OK during freeze;
this one qualifies.

Thanks,
Siddhesh

Patch
diff mbox series

diff --git a/malloc/Makefile b/malloc/Makefile
index 734efe368d..984045b5b9 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -27,7 +27,7 @@  headers := $(dist-headers) obstack.h mcheck.h
 tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-mcheck tst-mallocfork tst-trim1 \
 	 tst-malloc-usable tst-realloc tst-reallocarray tst-posix_memalign \
-	 tst-pvalloc tst-memalign tst-mallopt \
+	 tst-pvalloc tst-pvalloc-fortify tst-memalign tst-mallopt \
 	 tst-malloc-backtrace tst-malloc-thread-exit \
 	 tst-malloc-thread-fail tst-malloc-fork-deadlock \
 	 tst-mallocfork2 \
diff --git a/malloc/malloc.h b/malloc/malloc.h
index 0c76264421..a6903fdd54 100644
--- a/malloc/malloc.h
+++ b/malloc/malloc.h
@@ -71,8 +71,7 @@  extern void *valloc (size_t __size) __THROW __attribute_malloc__
 
 /* Equivalent to valloc(minimum-page-that-holds(n)), that is, round up
    __size to nearest pagesize. */
-extern void *pvalloc (size_t __size) __THROW __attribute_malloc__
-     __attribute_alloc_size__ ((1)) __wur;
+extern void *pvalloc (size_t __size) __THROW __attribute_malloc__ __wur;
 
 /* Underlying allocation function; successive calls should return
    contiguous pieces of memory.  */
diff --git a/malloc/tst-pvalloc-fortify.c b/malloc/tst-pvalloc-fortify.c
new file mode 100644
index 0000000000..391b7fa2f5
--- /dev/null
+++ b/malloc/tst-pvalloc-fortify.c
@@ -0,0 +1,48 @@ 
+/* Test fortify-source allocation size handling in pvalloc (bug 25401).
+   Copyright (C) 2020 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; see the file COPYING.LIB.  If
+   not, see <https://www.gnu.org/licenses/>.  */
+
+#undef _FORTIFY_SOURCE
+#define _FORTIFY_SOURCE 2
+#include <malloc.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static int
+do_test (void)
+{
+  /* The test below assumes that pvalloc rounds up the allocation size
+     to at least 8.  */
+  TEST_VERIFY (xsysconf (_SC_PAGESIZE) >= 8);
+
+  void *p = pvalloc (5);
+  TEST_VERIFY_EXIT (p != NULL);
+
+  /* This is valid assuming the page size is at least 8 because
+     pvalloc rounds up the allocation size to a multiple of the page
+     size.  Due to bug 25041, this used to trigger a compiler
+     warning.  */
+  strcpy (p, "abcdefg");
+
+  asm ("" : : "g" (p) : "memory"); /* Optimization barrier.  */
+  TEST_VERIFY (malloc_usable_size (p) >= xsysconf (_SC_PAGESIZE));
+  return 0;
+}
+
+#include <support/test-driver.c>