Re: [Take 2] winex11.drv: Fix buffer overflow bug in X11DRV_KeyEvent()and X11DRV_ToUnicodeEx()

[Dmitry Timoshkov]

"Muneyuki Noguchi"  wrote:
> Changes from the previous patch:
> - Add a new variable STRSIZE.

A define would be better IMO, or just use 64 directly, 2 places don't
justify the introduction of it.
> - Use HeapAlloc() and HeapFree() instead of malloc() and free().

I'd suggest to have an initial buffer of 64 bytes allocated on the stack,
and allocate a larger buffer only if required. That improves performance.
> - Return from a function when (re)allocation failed.

You have to call wine_tsx11_unlock() when needed on exit.

[Take 2] winex11.drv: Fix buffer overflow bug in X11DRV_KeyEvent()and X11DRV_ToUnicodeEx()

[Muneyuki Noguchi]

Thank you for your comments.
> I'd suggest to have an initial buffer of 64 bytes allocated on the stack,
> and allocate a larger buffer only if required. That improves performance.

lpChar and Str are referenced after XmbLookupString() or XLookupString() is called. If both a buffer on stack and a buffer on heap can be created, every time the buffer is referenced, I need to check whether the used buffer is one on stack or not:

if (isStack) {
   lpChar[0] = 0;
 } else {
   lpChar2[0] = 0;
 }

This implementation is ugly and won't improve performance so much.

[Take 2] winex11.drv: Fix buffer overflow bug in X11DRV_KeyEvent()and X11DRV_ToUnicodeEx()

[Dmitry Timoshkov]

"Muneyuki Noguchi"  wrote:
> > I'd suggest to have an initial buffer of 64 bytes allocated on the stack,
> > and allocate a larger buffer only if required. That improves performance.
>
> lpChar and Str are referenced after XmbLookupString() or XLookupString()
> is called. If both a buffer on stack and a buffer on heap can be created,
> every time the buffer is referenced, I need to check whether the used buffer
> is one on stack or not:
>
> if (isStack) {
>   lpChar[0] = 0;
> } else {
>   lpChar2[0] = 0;
> }
>
> This implementation is ugly and won't improve performance so much.

You only need to check if an actually used string buffer point to
the stack buffer before HeapFree().

char Str[64];
char *ptr = Str;

required_size = 64;

[get actual required_size]

if (required_size > 64)
    ptr = HeapAlloc(required_size);

[use ptr for the work]

if (ptr != Str)
    HeapFree(ptr);