Re: [PATCH] Refactor debug routines.

[Mike Frysinger <vapier at gentoo dot org>]

On Tuesday 22 October 2013 19:15:56 OndÅej BÃlka wrote:

random thought.  is there a reason we don't have routines check for overlaps ?  
e.g. memcpy spec says the regions must not overlap, so why don't we enforce 
that ?  seems better to have the code abort on arches even when it's safe to 
get better coverage when things happen to change ...

> --- a/debug/memcpy_chk.c
> +++ b/debug/memcpy_chk.c

OK

> --- a/debug/memmove_chk.c
> +++ b/debug/memmove_chk.c

OK

> --- a/debug/mempcpy_chk.c
> +++ b/debug/mempcpy_chk.c

OK

> --- a/debug/memset_chk.c
> +++ b/debug/memset_chk.c

OK

> --- a/debug/stpcpy_chk.c
> +++ b/debug/stpcpy_chk.c
> @@ -29,16 +29,7 @@ __stpcpy_chk (dest, src, destlen)
>       const char *src;
>       size_t destlen;
>  {
> -  char *d = dest;
> -  const char *s = src;
> -
> -  do
> -    {
> -      if (__builtin_expect (destlen-- == 0, 0))
> -	__chk_fail ();
> -      *d++ = *s;
> -    }
> -  while (*s++ != '\0');
> -
> -  return d - 1;
> +  size_t len = strlen (src) + 1;
> +  __memcpy_chk (dest, src, len, destlen);
> +  return dest + len - 1;
>  }

that strlen means you can scan the entire source string twice.  seems like 
this overall change might win in some cases, but lose in others.

i think it's safe to assume that the xxx_chk variants are becoming more common 
(i think all the major distros use it by default now) which means we need to 
be concerned with their performance.

that means if an arch wants this to perform, then they should implement it 
which includes the destlen check.

> --- a/debug/stpncpy_chk.c
> +++ b/debug/stpncpy_chk.c

OK

> --- a/debug/strcat_chk.c
> +++ b/debug/strcat_chk.c
> @@ -21,37 +21,16 @@
> 
>  /* Append SRC on the end of DEST.  */
>  char *
> -__strcat_chk (dest, src, destlen)
> +__strcat_chk (dest, src, totallen)
>       char *dest;
>       const char *src;
> -     size_t destlen;
> +     size_t totallen;

we should fix all these prototypes at some point ...

>  {
> -  char *s1 = dest;
> -  const char *s2 = src;
> -  char c;
> -
> -  /* Find the end of the string.  */
> -  do
> -    {
> -      if (__builtin_expect (destlen-- == 0, 0))
> -	__chk_fail ();
> -      c = *s1++;
> -    }
> -  while (c != '\0');
> ...
> +  size_t destlen = strlen (dest);

the dest strlen scan makes sense as we have to do that anyways (find the 
terminating NUL).  however, you'll notice that this new version lost a check 
the old one had -- what if dest string lacks a NUL within its bounds ?  yes, 
you would catch it below in your single check, but it might be too late by 
that point -- you might have scanned off into invalid memory and simply 
crashed.  i think you should use strnlen (dest, totalen) instead and check the 
return value explicitly.

> -  /* 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 (__builtin_expect (destlen-- == 0, 0))
> -	__chk_fail ();
> -      c = *s2++;
> -      *++s1 = c;
> -    }
> -  while (c != '\0');
> +  size_t srclen = strlen (src) + 1;
> +  if (__builtin_expect (totallen < destlen + srclen, 0))
> +	  __chk_fail ();

doesn't this suffer from possible wrap around ?  unlikely, but possible, unlike 
the previous code.  although, if you go with the suggestion below, this is a 
non-issue.

> +  memcpy (dest + destlen, src, srclen);
>    return dest;
>  }

we agree that you have to do strlen on the dest.  at that point, can't you 
call strcpy_chk to do the rest of the work ?  it avoids the 2nd strlen and the 
funky (overflowable) math.
	strcpy_chk(dest + destlen, src, totalen - destlen);
	return dest;

> --- a/debug/strcpy_chk.c
> +++ b/debug/strcpy_chk.c

same feedback as stpcpy_chk

> --- a/debug/strncat_chk.c
> +++ b/debug/strncat_chk.c

same feedback as strcat_chk, but this time use strncpy_chk:
	strncpy_chk(s1 + destlen, s2, n, s1len - destlen);
	return s1;

be nice to change the var names to "src" and "dest" and such like the existing 
strcat_chk func.

> --- a/debug/strncpy_chk.c
> +++ b/debug/strncpy_chk.c

OK

no comments about the x86_64 code
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.