[PATCH v2.1] Use saturated arithmetic for overflow detection.

[Joseph S. Myers]

On Tue, 3 Dec 2013, Ondrej Bilka wrote:

> @@ -1067,10 +1068,10 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>  	    /* Allocate dynamically an array which definitely is long	      \
>  	       enough for the wide character version.  Each byte in the	      \
>  	       multi-byte string can produce at most one wide character.  */  \
> -	    if (__libc_use_alloca (len * sizeof (wchar_t)))		      \
> -	      string = (CHAR_T *) alloca (len * sizeof (wchar_t));	      \
> -	    else if ((string = (CHAR_T *) malloc (len * sizeof (wchar_t)))    \
> -		     == NULL)						      \
> +	    size_t needed = mul_s (len, sizeof (wchar_t));		      \
> +	    if (__libc_use_alloca (needed))				      \
> +	      string = (CHAR_T *) alloca (needed);			      \
> +	    else if ((string = (CHAR_T *) malloc (needed)) == NULL)	      \

If this is fixing a bug with a missing overflow check, where valid (even 
if unusual) inputs could result in overflow, a corresponding bug should be 
filed in Bugzilla if not already present.  (Distribution security people 
can decide which such bugs are worth requesting CVEs for.)

> +static inline __attribute__((always_inline, unused)) size_t

Space before open parenthesis.  Function needs a comment explaining its 
semantics.

> +static inline __attribute__((always_inline, unused)) size_t
> +mul_s (size_t x, size_t y)

Likewise.

> +{
> +  if (__builtin_constant_p (x))
> +    {
> +      size_t tmp = x;
> +      x = y;
> +      y = tmp;
> +    }
> +  if (__builtin_constant_p (y))
> +    {
> +      if (y == 0)
> +        return 0;
> +      if (__glibc_unlikely (x > SIZE_MAX / y))
> +        return SIZE_MAX;
> +      return x * y;
> +    }

Have you verified that both the case of constant x and the case of 
constant y get optimized into a single comparison against a constant?  It 
would be less confusing anyway for each "if" to repeat the contents rather 
than swapping x and y, given that if __builtin_constant_p holds then only 
one "if" is going to be used anyway.

> +static inline __attribute__((always_inline, unused)) size_t
> +div_s (size_t x, size_t y)

I don't see the point of this function, and it's particularly confusing 
without properly documented semantics.

> @@ -619,14 +620,12 @@ gaih_inet (const char *name, const struct gaih_service *service,
>  		      if (i > 0 && *pat != NULL)
>  			--i;
>  
> -		      if (__libc_use_alloca (alloca_used
> -					     + i * sizeof (struct gaih_addrtuple)))
> -			addrmem = alloca_account (i * sizeof (struct gaih_addrtuple),
> -						  alloca_used);
> +		      size_t needed = mul_s (i,  sizeof (struct gaih_addrtuple));
> +		      if (__libc_use_alloca (alloca_used + needed))
> +			addrmem = alloca_account (needed, alloca_used);

Again, if fixing a user-visible bug (rather than simply reworking existing 
logic that already checks correctly for overflow) then please file it in 
Bugzilla.

-- 
Joseph S. Myers
joseph@codesourcery.com