Problem with Object Size Checking and reallocarray

Message ID 01d4ad3b-6708-69d2-f50e-8448982d16f9@dronecode.org.uk
State New
Headers show
Series
  • Problem with Object Size Checking and reallocarray
Related show

Commit Message

Jon Turney March 12, 2018, 7:58 p.m.
// gcc test.c -o test.exe -g -O2 -Wp,-D_FORTIFY_SOURCE=2

//
// extracted from InputLineAddChar in xserver/xkb/maprules.c
//

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main()
{
   const char *buf[128];
   char *line = reallocarray(NULL, 128, 2);
   // size of line is 128*2 = 256
   printf("%zu\n", __builtin_object_size(line, 0));
   memcpy(line, buf, 128);
   // __mempcy_chk tests against size 2, and terminates
}


reallocarray() is annotated in stdlib.h with '__alloc_size(2) 
__alloc_size(3)'

per [1], this doesn't seem to be the correct syntax when the size is the 
product of the arguments, and the last alloc_size seems to be silently 
winning.

If I change this to '__alloc_size((2,3))' (as in the patch attached), 
__builtin_object_size doesn't seem to be a compile-time constant 
anymore, and so memcpy() evaluates differently, so it's hard to be sure 
that's actually correct...

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
From 36fe1f9af2720247beb0dadaf089106faa85b453 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Mon, 12 Mar 2018 19:54:11 +0000
Subject: [PATCH] Correct alloc_size annotation on reallocarray()

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 newlib/libc/include/stdlib.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Corinna Vinschen March 13, 2018, 12:51 p.m. | #1
On Mar 12 19:58, Jon Turney wrote:
> 

> // gcc test.c -o test.exe -g -O2 -Wp,-D_FORTIFY_SOURCE=2

> 

> //

> // extracted from InputLineAddChar in xserver/xkb/maprules.c

> //

> 

> #include <stdio.h>

> #include <stdlib.h>

> #include <string.h>

> 

> int main()

> {

>   const char *buf[128];

>   char *line = reallocarray(NULL, 128, 2);

>   // size of line is 128*2 = 256

>   printf("%zu\n", __builtin_object_size(line, 0));

>   memcpy(line, buf, 128);

>   // __mempcy_chk tests against size 2, and terminates

> }

> 

> 

> reallocarray() is annotated in stdlib.h with '__alloc_size(2)

> __alloc_size(3)'

> 

> per [1], this doesn't seem to be the correct syntax when the size is the

> product of the arguments, and the last alloc_size seems to be silently

> winning.

> 

> If I change this to '__alloc_size((2,3))' (as in the patch attached),

> __builtin_object_size doesn't seem to be a compile-time constant anymore,

> and so memcpy() evaluates differently, so it's hard to be sure that's

> actually correct...

> 

> [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html


Yaakov, care to comment and push if the patch is ok?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Yaakov Selkowitz March 13, 2018, 2:05 p.m. | #2
On 2018-03-13 07:51, Corinna Vinschen wrote:
> On Mar 12 19:58, Jon Turney wrote:

>> reallocarray() is annotated in stdlib.h with '__alloc_size(2)

>> __alloc_size(3)'

>>

>> per [1], this doesn't seem to be the correct syntax when the size is the

>> product of the arguments, and the last alloc_size seems to be silently

>> winning.

>>

>> If I change this to '__alloc_size((2,3))' (as in the patch attached),

>> __builtin_object_size doesn't seem to be a compile-time constant anymore,

>> and so memcpy() evaluates differently, so it's hard to be sure that's

>> actually correct...

>>

>> [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

> 

> Yaakov, care to comment and push if the patch is ok?


Thanks, pushed.

-- 
Yaakov
Richard Earnshaw (lists) March 13, 2018, 6:28 p.m. | #3
On 13/03/18 14:05, Yaakov Selkowitz wrote:
> On 2018-03-13 07:51, Corinna Vinschen wrote:

>> On Mar 12 19:58, Jon Turney wrote:

>>> reallocarray() is annotated in stdlib.h with '__alloc_size(2)

>>> __alloc_size(3)'

>>>

>>> per [1], this doesn't seem to be the correct syntax when the size is the

>>> product of the arguments, and the last alloc_size seems to be silently

>>> winning.

>>>

>>> If I change this to '__alloc_size((2,3))' (as in the patch attached),

>>> __builtin_object_size doesn't seem to be a compile-time constant anymore,

>>> and so memcpy() evaluates differently, so it's hard to be sure that's

>>> actually correct...

>>>

>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

>>

>> Yaakov, care to comment and push if the patch is ok?

> 

> Thanks, pushed.

> 


This doesn't work at all.  I get:

warning: alloc_size parameter outside range [-Wattributes]

There's too many levels of parenthesis around the parameters, so it
expands to
void *reallocarray(void *, size_t, size_t)
__attribute__((__alloc_size__((2,3))));

And this causes all the testsuites to start failing due tot he warning.

To silence the warning it needs to be __attribute__((__alloc_size__(2,3)))

I'm not sure how you achieve that, given the macro expansion going on here.

R.
Yaakov Selkowitz March 13, 2018, 8:54 p.m. | #4
On 2018-03-13 13:28, Richard Earnshaw (lists) wrote:
> On 13/03/18 14:05, Yaakov Selkowitz wrote:

>> On 2018-03-13 07:51, Corinna Vinschen wrote:

>>> On Mar 12 19:58, Jon Turney wrote:

>>>> reallocarray() is annotated in stdlib.h with '__alloc_size(2)

>>>> __alloc_size(3)'

>>>>

>>>> per [1], this doesn't seem to be the correct syntax when the size is the

>>>> product of the arguments, and the last alloc_size seems to be silently

>>>> winning.

>>>>

>>>> If I change this to '__alloc_size((2,3))' (as in the patch attached),

>>>> __builtin_object_size doesn't seem to be a compile-time constant anymore,

>>>> and so memcpy() evaluates differently, so it's hard to be sure that's

>>>> actually correct...

>>>>

>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

>>>

>>> Yaakov, care to comment and push if the patch is ok?

>>

>> Thanks, pushed.

>>

> 

> This doesn't work at all.  I get:

> 

> warning: alloc_size parameter outside range [-Wattributes]

> 

> There's too many levels of parenthesis around the parameters, so it

> expands to

> void *reallocarray(void *, size_t, size_t)

> __attribute__((__alloc_size__((2,3))));

> 

> And this causes all the testsuites to start failing due tot he warning.

> 

> To silence the warning it needs to be __attribute__((__alloc_size__(2,3)))

> 

> I'm not sure how you achieve that, given the macro expansion going on here.


Does the attached help?

-- 
Yaakov
diff --git a/newlib/libc/include/stdlib.h b/newlib/libc/include/stdlib.h
index 593760a12..564ce8a28 100644
--- a/newlib/libc/include/stdlib.h
+++ b/newlib/libc/include/stdlib.h
@@ -324,8 +324,8 @@ extern long double strtold (const char *__restrict, char **__restrict);
  * If we're in a mode greater than C99, expose C11 functions.
  */
 #if __ISO_C_VISIBLE >= 2011
-void *	aligned_alloc(size_t, size_t) __malloc_like __alloc_align(1)
-	    __alloc_size(2);
+void *	aligned_alloc(size_t, size_t) __malloc_like __alloc_align((1))
+	    __alloc_size((2));
 int	at_quick_exit(void (*)(void));
 _Noreturn void
 	quick_exit(int);
diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
index fc564a5c6..b3f8d1965 100644
--- a/newlib/libc/include/sys/cdefs.h
+++ b/newlib/libc/include/sys/cdefs.h
@@ -258,12 +258,12 @@
 #define	__section(x)	__attribute__((__section__(x)))
 #endif
 #if __GNUC_PREREQ__(4, 3) || __has_attribute(__alloc_size__)
-#define	__alloc_size(x)	__attribute__((__alloc_size__(x)))
+#define	__alloc_size(x)	__attribute__((__alloc_size__ x))
 #else
 #define	__alloc_size(x)
 #endif
 #if __GNUC_PREREQ__(4, 9) || __has_attribute(__alloc_align__)
-#define	__alloc_align(x)	__attribute__((__alloc_align__(x)))
+#define	__alloc_align(x)	__attribute__((__alloc_align__ x))
 #else
 #define	__alloc_align(x)
 #endif
Richard Earnshaw (lists) March 14, 2018, 11:34 a.m. | #5
On 13/03/18 20:54, Yaakov Selkowitz wrote:
> On 2018-03-13 13:28, Richard Earnshaw (lists) wrote:

>> On 13/03/18 14:05, Yaakov Selkowitz wrote:

>>> On 2018-03-13 07:51, Corinna Vinschen wrote:

>>>> On Mar 12 19:58, Jon Turney wrote:

>>>>> reallocarray() is annotated in stdlib.h with '__alloc_size(2)

>>>>> __alloc_size(3)'

>>>>>

>>>>> per [1], this doesn't seem to be the correct syntax when the size is the

>>>>> product of the arguments, and the last alloc_size seems to be silently

>>>>> winning.

>>>>>

>>>>> If I change this to '__alloc_size((2,3))' (as in the patch attached),

>>>>> __builtin_object_size doesn't seem to be a compile-time constant anymore,

>>>>> and so memcpy() evaluates differently, so it's hard to be sure that's

>>>>> actually correct...

>>>>>

>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

>>>>

>>>> Yaakov, care to comment and push if the patch is ok?

>>>

>>> Thanks, pushed.

>>>

>>

>> This doesn't work at all.  I get:

>>

>> warning: alloc_size parameter outside range [-Wattributes]

>>

>> There's too many levels of parenthesis around the parameters, so it

>> expands to

>> void *reallocarray(void *, size_t, size_t)

>> __attribute__((__alloc_size__((2,3))));

>>

>> And this causes all the testsuites to start failing due tot he warning.

>>

>> To silence the warning it needs to be __attribute__((__alloc_size__(2,3)))

>>

>> I'm not sure how you achieve that, given the macro expansion going on here.

> 

> Does the attached help?


Yes, that seems to fix it.

Thanks for the quick turn-around.

R.

> 

> 

> 0001-alloc-macros.patch

> 

> 

> diff --git a/newlib/libc/include/stdlib.h b/newlib/libc/include/stdlib.h

> index 593760a12..564ce8a28 100644

> --- a/newlib/libc/include/stdlib.h

> +++ b/newlib/libc/include/stdlib.h

> @@ -324,8 +324,8 @@ extern long double strtold (const char *__restrict, char **__restrict);

>   * If we're in a mode greater than C99, expose C11 functions.

>   */

>  #if __ISO_C_VISIBLE >= 2011

> -void *	aligned_alloc(size_t, size_t) __malloc_like __alloc_align(1)

> -	    __alloc_size(2);

> +void *	aligned_alloc(size_t, size_t) __malloc_like __alloc_align((1))

> +	    __alloc_size((2));

>  int	at_quick_exit(void (*)(void));

>  _Noreturn void

>  	quick_exit(int);

> diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h

> index fc564a5c6..b3f8d1965 100644

> --- a/newlib/libc/include/sys/cdefs.h

> +++ b/newlib/libc/include/sys/cdefs.h

> @@ -258,12 +258,12 @@

>  #define	__section(x)	__attribute__((__section__(x)))

>  #endif

>  #if __GNUC_PREREQ__(4, 3) || __has_attribute(__alloc_size__)

> -#define	__alloc_size(x)	__attribute__((__alloc_size__(x)))

> +#define	__alloc_size(x)	__attribute__((__alloc_size__ x))

>  #else

>  #define	__alloc_size(x)

>  #endif

>  #if __GNUC_PREREQ__(4, 9) || __has_attribute(__alloc_align__)

> -#define	__alloc_align(x)	__attribute__((__alloc_align__(x)))

> +#define	__alloc_align(x)	__attribute__((__alloc_align__ x))

>  #else

>  #define	__alloc_align(x)

>  #endif

>

Patch

diff --git a/newlib/libc/include/stdlib.h b/newlib/libc/include/stdlib.h
index ef0e4bdda..593760a12 100644
--- a/newlib/libc/include/stdlib.h
+++ b/newlib/libc/include/stdlib.h
@@ -140,8 +140,7 @@  void	qsort (void *__base, size_t __nmemb, size_t __size, __compar_fn_t _compar);
 int	rand (void);
 void *	realloc (void *__r, size_t __size) _NOTHROW;
 #if __BSD_VISIBLE
-void	*reallocarray(void *, size_t, size_t) __result_use_check __alloc_size(2)
-	    __alloc_size(3);
+void	*reallocarray(void *, size_t, size_t) __result_use_check __alloc_size((2,3));
 void *	reallocf (void *__r, size_t __size);
 #endif
 #if __BSD_VISIBLE || __XSI_VISIBLE >= 4