Re: [PATCH] Refactor debug routines.

[Mike Frysinger <vapier at gentoo dot org>]

On Tuesday 03 December 2013 20:39:53 OndÅej BÃlka wrote:
> 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:
> > > --- 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 ();

every once in a while i run into situations where i wish there were more stp 
variants of existing funcs.  maybe worth proposing as GNU extensions ?

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

one of the nice things about fortify is that it provides nice abort messages 
rather than just a segfault.  the former are generally taken pretty seriously 
while the latter are sometimes shrugged off as random flakes.

if the performance degradation is significant, i could live with your variant.
-mike

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