Re: [PATCH] Refactor debug routines.

[OndÅej BÃlka <neleai at seznam dot cz>]

On Sat, Nov 30, 2013 at 02:24:12AM -0500, Mike Frysinger wrote:
> 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 ...
>
I send patch for that earlier, these check will be easier to add with
refactoring. 

To reduce size I pushed acked parts,  

> > --- 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.
>
As this is at most twice slower than nonchecking variant its relatively
reasonable. Worse problem was x64 assembly code which was based on old
implementation and now is around five times slower than this code.

A one-pass implementation could be done if we add a stpcat/stpncat functions that
returns end of string, these are usefull on their own as it make easier
avoid quadratic loops by concenating strings / letting gcc use that optimization.

dest[0] = '\0';
char *end = stpncat (dest, src, destlen);
if (end - dest == destlen)
  __chk_fail ();

> >  {
> > -  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.
>
I did that for performance reasons as segfault is similarly easy to
debug. A strnlen would only mostly work, bound here is not tight, if you
have

if (c)
  x = malloc (100);
else
  x = malloc (1000);

then 1000 will be used as bound.

> > -  /* 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;
>

This is possible. 

> > --- 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