On Thu, 4 Sep 2008, Marcus Meissner wrote: [...]
+#if defined(__GNUC__) && (__GNUC__ < 4) +# define __builtin_object_size(x,y) -1 +#endif
Shouldn't this be:
#if !defined(__GNUC__) || (__GNUC__ < 4)
To preserve compatibility with non-GNU compilers.
+static inline INT +WINAPI MultiByteToWideChar_ichk(
- UINT cp,DWORD flags,
- LPCSTR src,INT srclen,INT srcbuflen,
- LPWSTR dst,INT dstlen,INT dstbuflen
+) {
- if ((srclen != -1) && (srcbuflen != -1) && (srcbuflen < srclen))
RaiseException(0xC0000409,0x01,0,NULL);
It would be nice to issue a trace clarifying the cause of the exception before raising it. Maybe something like this:
TRACE("used %d as the size when >= %d is needed for the builtin object\n", ...);
Sending it to the seh channel would be best.
[...]
+#define MultiByteToWideChar(cp,flags,src,srclen,dst,dstlen) \
- MultiByteToWideChar_ichk(cp,flags, \
src,srclen,__builtin_object_size(src,0), \dst,dstlen,__builtin_object_size(dst,0) \- )
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. * It's customary to add some extra parenthesing in such macros: (src),(srclen),__builtin_object_size((src),0), * 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? * 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.
2008/9/5 Francois Gouget fgouget@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.
- It's customary to add some extra parenthesing in such macros: (src),(srclen),__builtin_object_size((src),0),
- 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?
- 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.
I also have some questions: * Is there any runtime overhead of using __builtin_object_size and/or the __alloc_size__ attribute? * 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)?
On Fri, Sep 05, 2008 at 10:44:50PM +0100, Rob Shearman wrote:
2008/9/5 Francois Gouget fgouget@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
-
Francois Gouget -
Marcus Meissner -
Rob Shearman