[PATCH] buffer overflow checking for string functions

[Francois Gouget]

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.


-- 
Francois Gouget <fgouget at free.fr>              http://fgouget.free.fr/
                      Computers are like airconditioners
                They stop working properly if you open WINDOWS