Improve str(n)cat_chk performance.

  Here we sync implementation with strcat/strncat generic one. That makes
performance comparable with strcat instead of slow byte-by-byte loop.

Ok to commit this?


	* debug/strcat_chk.c (__strcat_chk): Sync with generic implementation.
	* debug/strncat_chk.c (__strncat_chk): Likewise.
  

ping
On Thu, Aug 20, 2015 at 09:35:17AM +0200, Ondřej Bílka wrote:
> Here we sync implementation with strcat/strncat generic one. That makes
> performance comparable with strcat instead of slow byte-by-byte loop.
> 
> Ok to commit this?
> 
> 
> 	* debug/strcat_chk.c (__strcat_chk): Sync with generic implementation.
> 	* debug/strncat_chk.c (__strncat_chk): Likewise.
> 
> diff --git a/debug/strcat_chk.c b/debug/strcat_chk.c
> index 8d7359f..8f26012 100644
> --- a/debug/strcat_chk.c
> +++ b/debug/strcat_chk.c
> @@ -16,8 +16,6 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <string.h>
> -#include <memcopy.h>
> -
>  
>  /* Append SRC on the end of DEST.  */
>  char *
> @@ -26,32 +24,12 @@ __strcat_chk (dest, src, destlen)
>       const char *src;
>       size_t destlen;
>  {
> -  char *s1 = dest;
> -  const char *s2 = src;
> -  char c;
> -
> -  /* Find the end of the string.  */
> -  do
> -    {
> -      if (__glibc_unlikely (destlen-- == 0))
> -	__chk_fail ();
> -      c = *s1++;
> -    }
> -  while (c != '\0');
> -
> -  /* Make S1 point before the next character, so we can increment
> -     it while memory is read (wins on pipelined cpus).  */
> -  ++destlen;
> -  s1 -= 2;
> -
> -  do
> -    {
> -      if (__glibc_unlikely (destlen-- == 0))
> -	__chk_fail ();
> -      c = *s2++;
> -      *++s1 = c;
> -    }
> -  while (c != '\0');
> +  size_t len = strlen (dest);
> +  size_t srclen = strlen (src);
>  
> +  if (__glibc_unlikely (len + srclen + 1 > destlen))
> +    __chk_fail ();
> +  
> +  memcpy (dest + len, src, srclen + 1);
>    return dest;
>  }
> diff --git a/debug/strncat_chk.c b/debug/strncat_chk.c
> index 2951d05..c0a68c5 100644
> --- a/debug/strncat_chk.c
> +++ b/debug/strncat_chk.c
> @@ -17,83 +17,16 @@
>  
>  #include <string.h>
>  
> -#include <memcopy.h>
> -
> -
>  char *
> -__strncat_chk (s1, s2, n, s1len)
> -     char *s1;
> -     const char *s2;
> -     size_t n;
> -     size_t s1len;
> +__strncat_chk (char *dest, const char *src, size_t n, size_t destlen)
>  {
> -  char c;
> -  char *s = s1;
> -
> -  /* Find the end of S1.  */
> -  do
> -    {
> -      if (__glibc_unlikely (s1len-- == 0))
> -	__chk_fail ();
> -      c = *s1++;
> -    }
> -  while (c != '\0');
> -
> -  /* Make S1 point before next character, so we can increment
> -     it while memory is read (wins on pipelined cpus).  */
> -  ++s1len;
> -  s1 -= 2;
> -
> -  if (n >= 4)
> -    {
> -      size_t n4 = n >> 2;
> -      do
> -	{
> -	  if (__glibc_unlikely (s1len-- == 0))
> -	    __chk_fail ();
> -	  c = *s2++;
> -	  *++s1 = c;
> -	  if (c == '\0')
> -	    return s;
> -	  if (__glibc_unlikely (s1len-- == 0))
> -	    __chk_fail ();
> -	  c = *s2++;
> -	  *++s1 = c;
> -	  if (c == '\0')
> -	    return s;
> -	  if (__glibc_unlikely (s1len-- == 0))
> -	    __chk_fail ();
> -	  c = *s2++;
> -	  *++s1 = c;
> -	  if (c == '\0')
> -	    return s;
> -	  if (__glibc_unlikely (s1len-- == 0))
> -	    __chk_fail ();
> -	  c = *s2++;
> -	  *++s1 = c;
> -	  if (c == '\0')
> -	    return s;
> -	} while (--n4 > 0);
> -      n &= 3;
> -    }
> -
> -  while (n > 0)
> -    {
> -      if (__glibc_unlikely (s1len-- == 0))
> -	__chk_fail ();
> -      c = *s2++;
> -      *++s1 = c;
> -      if (c == '\0')
> -	return s;
> -      n--;
> -    }
> +  size_t len = strlen (dest);
> +  size_t srclen = __strnlen (src, n);
>  
> -  if (c != '\0')
> -    {
> -      if (__glibc_unlikely (s1len-- == 0))
> -	__chk_fail ();
> -      *++s1 = '\0';
> -    }
> +  if (__glibc_unlikely (len + srclen + 1 > destlen))
> +    __chk_fail ();
>  
> -  return s;
> +  dest[len + srclen] = 0;
> +  memcpy (dest + len, src, srclen);
> +  return dest;
>  }
  

On 26-08-2015 13:43, Ondřej Bílka wrote:
> ping

>> -  while (c != '\0');
>> +  size_t len = strlen (dest);
>> +  size_t srclen = strlen (src);
>>  
>> +  if (__glibc_unlikely (len + srclen + 1 > destlen))
>> +    __chk_fail ();

Couldn't this test potentially wrap over the unsigned operation?  I think
we should add an extra test to avoid this:

/* Check if the unsigned operation do not wrap.  */
if (__glibc_unlikely(SIZE_MAX - (srclen + 1) < len))
  __chk_fail ();
if (__glibc_unlikely(len + srclen + 1 > destlen))
  __chk_fail ();

Same applies for strncat_chk.

>> +  if (__glibc_unlikely (len + srclen + 1 > destlen))
>> +    __chk_fail ();
>>  
>> -  return s;
>> +  dest[len + srclen] = 0;
>> +  memcpy (dest + len, src, srclen);
>> +  return dest;
>>  }
>
  

On Wed, Aug 26, 2015 at 05:52:39PM -0300, Adhemerval Zanella wrote:
> 
> 
> On 26-08-2015 13:43, Ondřej Bílka wrote:
> > ping
> 
> >> -  while (c != '\0');
> >> +  size_t len = strlen (dest);
> >> +  size_t srclen = strlen (src);
> >>  
> >> +  if (__glibc_unlikely (len + srclen + 1 > destlen))
> >> +    __chk_fail ();
> 
> Couldn't this test potentially wrap over the unsigned operation?  I think
> we should add an extra test to avoid this:
>
As this calculates size of physical memory occupied such computations couldn't overflow unless there is overlap.

I send patch to also test overlaps in past I could dig it.

That would be better than adding checks for corner cases when user needs
to allocate string larger than half of address space and call strcat
with that one as both arguments.