[PATCH xserver] dix: Don't leak swapped events

[Peter Hutterer]

On Tue, Mar 28, 2017 at 01:42:21PM -0400, Adam Jackson wrote:
> The original code here is silly. Just allocate an event on the stack and
> swap into that instead of forcing an ever-growing array into the heap.
> 
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  dix/events.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/dix/events.c b/dix/events.c
> index cc26ba5..fd3434f 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -266,9 +266,6 @@ static struct DeviceEventTime {
>   */
>  #define RootWindow(sprite) sprite->spriteTrace[0]
>  
> -static xEvent *swapEvent = NULL;
> -static int swapEventLen = 0;
> -
>  void
>  NotImplemented(xEvent *from, xEvent *to)
>  {
> @@ -5897,7 +5894,6 @@ WriteEventsToClient(ClientPtr pClient, int count, xEvent *events)
>  #ifdef PANORAMIX
>      xEvent eventCopy;
>  #endif
> -    xEvent *eventTo, *eventFrom;
>      int i, eventlength = sizeof(xEvent);
>  
>      if (!pClient || pClient == serverClient || pClient->clientGone)
> @@ -5972,25 +5968,16 @@ WriteEventsToClient(ClientPtr pClient, int count, xEvent *events)
>      }
>  
>      if (pClient->swapped) {
> -        if (eventlength > swapEventLen) {
> -            swapEventLen = eventlength;
> -            swapEvent = realloc(swapEvent, swapEventLen);
> -            if (!swapEvent) {
> -                FatalError("WriteEventsToClient: Out of memory.\n");
> -                return;
> -            }
> -        }
> -
>          for (i = 0; i < count; i++) {
> -            eventFrom = &events[i];
> -            eventTo = swapEvent;
> +            xEvent swapped;

as keith already pointed out, this will break for generic events. but you
can still get the same effect by switching to
   unsigned char buffer[eventlength];
   xEvent *swapped = buffer;
and get rid of the heap allocation this way.

>  
> +            memset(&swapped, 0, sizeof swapped);
>              /* Remember to strip off the leading bit of type in case
>                 this event was sent with "SendEvent." */
> -            (*EventSwapVector[eventFrom->u.u.type & 0177])
> -                (eventFrom, eventTo);
> +            (*EventSwapVector[events[i].u.u.type & 0177])
> +                (&events[i], &swapped);

I really dislike the removal of eventTo and eventFrom. Having a function
call foo(to, from) makes it bleedingly obvious if the arguments are mixed,
aside from documenting what foo probably does. Compare that to foo(&a[i],
bar), which tells me nothing.

Given enough compiler optimisation, those variables are free, we should aim
for legibility and obviousness here.

Cheers,
   Peter

>  
> -            WriteToClient(pClient, eventlength, eventTo);
> +            WriteToClient(pClient, eventlength, &swapped);
>          }
>      }
>      else {
> -- 
> 2.9.3