Re: [RFC][PATCH v2] Initial support for C11 Annex K Bounds checking functions

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 5 Jun 2013, Ulrich Bayer wrote:

> diff --git a/include/features.h b/include/features.h
> index ca83da0..f9b805d 100644
> --- a/include/features.h
> +++ b/include/features.h
> @@ -45,7 +45,8 @@
>     _THREAD_SAFE		Same as _REENTRANT, often used by other systems.
>     _FORTIFY_SOURCE	If set to numeric value > 0 additional security
>  			measures are defined, according to level.
> -
> +   __STDC_WANT_LIB_EXT1__ Includes bounds checking functions (with postfix _s)
> +                          specified in Annex K of ISO C11.
>     The `-ansi' switch to the GNU C compiler defines __STRICT_ANSI__.
>     If none of these are defined, the default is to have _SVID_SOURCE,
>     _BSD_SOURCE, and _POSIX_SOURCE set to one and _POSIX_C_SOURCE set to
> @@ -81,6 +82,8 @@
>     __USE_REENTRANT	Define reentrant/thread-safe *_r functions.
>     __USE_FORTIFY_LEVEL	Additional security measures used, according to level.
>     __FAVOR_BSD		Favor 4.3BSD things in cases of conflict.
> +   __USE_BOUNDS_CHECKING Define the bounds checking interfaces from
> +                         C11 Annex K.

The definition of __STDC_WANT_LIB_EXT1__ is that it is only relevant on 
inclusion of a header in K.3 (that is, <errno.h>, <stddef.h>, <stdint.h>, 
<stdio.h>, <stdlib.h>, <string.h>, <time.h>, <wchar.h>) - and not relevant 
on inclusion of any other ISO C header.

Thus, checking it only in <features.h>, then using __USE_BOUNDS_CHECKING 
in other headers, is incorrect.  Instead, the relevant headers need to 
check __STDC_WANT_LIB_EXT1__ directly.  The only thing <features.h> can 
usefully do is to define __STDC_WANT_LIB_EXT1__ if the user defined 
_GNU_SOURCE (if we want these interfaces to be part of the _GNU_SOURCE API 
- and normally all interfaces do go in that API).

> +CFLAGS-libc_s := -D__STDC_WANT_LIB_EXT1__=1

If you include it in _GNU_SOURCE, this should no longer be necessary.

> diff --git a/lib_s/abort_handler_s.c b/lib_s/abort_handler_s.c
> new file mode 100644
> index 0000000..75b7fef
> --- /dev/null
> +++ b/lib_s/abort_handler_s.c
> @@ -0,0 +1,38 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.

The first line of each new file should be a descriptive comment.  The 
copyright notice comes after that description of the file.  (Comment 
applies to other files as well.)

> +void
> +abort_handler_s (const char *restrict msg, void *restrict ptr, errno_t error)

Please include a comment on each function definition with a brief 
description of the function semantics and those of the parameters, as 
described in the GNU Coding Standards.  (Likewise for other functions.)

> +/* The strcpy_s function copies the string s2 to s1.
> +   Unlike the traditional strcpy, the size of the destination buffer s1
> +   has to be specified. The function makes sure that it never
> +   writes outside the specified bounds.
> +*/
> +extern errno_t strcpy_s (char * __restrict__ s1, rsize_t s1max,
> +			 const char * __restrict__ s2)
> +  __THROW __nonnull ((1,3));

I don't think it's appropriate to use __THROW on any function with runtime 
constraints; it's reasonable for someone to set a runtime constraint 
handler that throws C++ exceptions.

Likewise, it's inappropriate to use the nonnull attribute on arguments 
where the semantics if those arguments are NULL is specified (as a runtime 
constraint violation), because GCC may optimize based on the argument not 
being NULL - the attribute can be used only when a NULL argument means 
undefined behavior.

(Likewise for other functions affected.)

> +   "For implementations targeting machines with large address spaces,
> +   it is recommended that RSIZE_MAX be defined as the smaller of the
> +   size of the largest object supported or(SIZE_MAX >> 1), even if this

Missing space between "or" and '('.

-- 
Joseph S. Myers
joseph@codesourcery.com