[PATCH] buffer overflow checking for string functions
marcus at rennboot.centrumbabylon.cz
Sun Sep 7 15:15:11 CDT 2008
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
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 ;)
More information about the wine-devel