Bug 676270

Created attachment 477775 [details]
Proposed patch

Description of problem:

Our customer is experiencing random Xserver crashes with backing store enabled.

Version-Release number of selected component (if applicable):

xorg-x11-server-Xorg-1.1.1-48.41.el5_2.1

How reproducible:

Randomly

Steps to Reproduce:

Unknown
  
Actual results:

Xserver crashes with:

#0  0x00002b1fe2f83155 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00002b1fe2f84bf0 in abort () at abort.c:88
#2  0x0000000000460cd3 in ddxGiveUp () at xf86Init.c:1261
#3  0x000000000055d753 in AbortServer () at log.c:408
#4  0x000000000055dd95 in FatalError (f=0x570bb8 "Caught signal %d.  Server aborting\n") at log.c:554
#5  0x00000000004a107c in xf86SigHandler (signo=11) at xf86Events.c:1484
#6  <signal handler called>
#7  miRegionDestroy (pReg=0x0) at miregion.c:397
#8  0x00000000004dda98 in miSetShape (pWin=0x1b8fe140) at miwindow.c:1052
#9  0x00000000004e7cf7 in RegionOperate (client=0x1b635a70, pWin=0x1b8fe140, kind=0, destRgnp=0x1b9b8468, srcRgn=0x1b73f370, op=0, xoff=0, yoff=0, create=0x4e76e0 <CreateBoundingShape>) at shape.c:255
#10 0x00000000004e7ee2 in ProcShapeRectangles (client=0x1b635a70) at shape.c:379
#11 0x0000000000449c9a in Dispatch () at dispatch.c:459
#12 0x00000000004325ee in main (argc=8, argv=0x7fffc8ca8698, envp=<value optimized out>) at main.c:447

Expected results:

Xserver does not crash

Additional info:

The crash occurs because miSetShape() is calling miRegionDestroy() with a NULL region. The question is why this happens :)

The code looks like this:
   
    984    _X_EXPORT void
    985    miSetShape(pWin)
    986        register WindowPtr        pWin;
    987    {
    ...
    991        RegionPtr        pOldClip = NULL, bsExposed;
    ...
    1021       if (WasViewable)
    1022       {
    1023           if (pWin->backStorage)
    1024           {
    1025               pOldClip = REGION_CREATE(pScreen, NullBox, 1);
    1026               REGION_COPY(pScreen, pOldClip, &pWin->clipList);
    1027           }
    1028   
    ...
    1032   #ifdef DO_SAVE_UNDERS
    1033           if (DO_SAVE_UNDERS(pWin))
    1034           {
    1035               dosave = (*pScreen->ChangeSaveUnder)(pLayerWin, pLayerWin);
    1036           }
    1037   #endif /* DO_SAVE_UNDERS */
    ...
    1043       if (pWin->backStorage &&
    1044           ((pWin->backingStore == Always) || WasViewable))
    1045       {
    1046           if (!WasViewable)
    1047               pOldClip = &pWin->clipList; /* a convenient empty region */
    1048           bsExposed = (*pScreen->TranslateBackingStore)
    1049                                (pWin, 0, 0, pOldClip,
    1050                                 pWin->drawable.x, pWin->drawable.y);
    1051           if (WasViewable)
    1052               REGION_DESTROY(pScreen, pOldClip);
    ...

The crash occurs here at line 1052 ^^^ because pOldClip is NULL.

We see that pOldClip is initialized to NULL (line 991). 

We know that at line 1023 pWin->backStorage was null, otherwise pOldClip would have been created.

Yet, line 1043 pWin->backStorage is not NULL anymore (otherwise we would not go into the portion of code that fails). This is also confirmed by the corefile:

    (gdb) p pWin->backStorage
    $2 = (pointer) 0x1b812280

So the code wrongly assume that if pWin->backStorage is NULL at line 1023 it is still NULL at line 1043.

This is wrong because pWin->backStorage can be set in ChangeSaveUnder() (called line 1035) and ChangeSaveUnder() translates to miChangeSaveUnderwhich end up calling miBSChangeWindowAttributes() which sets pWin->backStorage.

So the code should check for the value of pOldClip line 1051 as it could be still NULL even if pWin->backStorage was NULL ealier on, it does not mean it is still the case line 1043.

Hence the proposed patch (trivial patch actually).