[PATCH] Refactor debug routines.

[Ondřej Bílka]

On Mon, Dec 23, 2013 at 03:03:30AM -0500, Mike Frysinger wrote:
> 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:
> > > 
> > > 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 ?
>
I could send that in 2.20.

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

I wrote that on assumption that these are equivalent where it would be a
performance degradation with no reason. With that information even more
expensive checks become feasible.