[PATCH] buffer overflow checking for string functions

[Marcus Meissner]

On Fri, Sep 05, 2008 at 10:44:50PM +0100, Rob Shearman wrote:
> 2008/9/5 Francois Gouget <fgouget at free.fr>:
> > I have a few other concerns here:
> >  * Why do we need a macro here? I thought it was so that
> >   __builtin_object_size() could do its work, but the strcpy() functions
> >   above in the patch have no associated macro and they call
> >   __builtin_object_size() on their parameter which would seem to be
> >   poinless. So I must be misunderstanding something here.

The strcpy functions there are inline, so we can modify the inline version.

strcpy() itself is replaced by glibc headers (bits/string2.h and string3.h)
into either a __builtin_strcpy_chk call, which gcc knows how to handle.
(in earlier glibc/gcc versions with an additional macro wrapper).


> >  * It's customary to add some extra parenthesing in such macros:
> >      (src),(srclen),__builtin_object_size((src),0),

Yes, need to fix.

> >  * The macro itself can cause trouble in some cases. In Winelib
> >   code it can cause naming collisions with user functions (especially
> >   with class methods, though it may not be very likely here), which
> >   would especially be an issue with strcpy*(). Maybe it should be
> >   protected by something like a WINE_NO_OVERFLOW_CHECK macro?

So far we have no problems in Wine sourcecode. I could easily limit
it to be __WINESRC__ only.

> >  * The macro can also cause trouble in case its parameters have
> >   side-effects, like x++ or similar (though the __builtin_object_size()
> >   mentions returning -1 in case or side-effects). This could impact
> >   Wine too.

Need to fix, yes.

> I also have some questions:
> * Is there any runtime overhead of using __builtin_object_size and/or
> the __alloc_size__ attribute?

There is no runtime overhead.

__alloc_size__ is applied at compile time only for constant size
allocations (and carried along with the pointer inside GCC during compile).

__builtin_object_size is evaluated at compile time too. If it cannot
be determined, it will evaluate to -1.

> * If not and it's a compile time only thing, why can't these buffer
> overruns be detected at compile time instead of runtime (which
> obviously depends on test coverage)?

If you have a known-size destination buffer, but a variable source buffer
this cannot be detected.

If both are known-size at compile time, a compilation abort should be
possible.

Hmm, parts of the MBtoWC check at least could be made to fail at compile time,
like the bug I fixed some days ago.

This patch needs to go back to the drawing board ;)

Ciao, Marcus