Am 20.11.2011 12:48, schrieb Jamey Sharp: > By using the same code for both MakeWindowOptional and CreateRootWindow, > we ensure that new WindowOptPtrs are always fully initialized. > > Previously, CreateRootWindow did not initialize optional->cursor. It > relied on InitRootWindow to overwrite it later, which seems like a wild > pointer dereference just waiting to happen. > > Signed-off-by: Jamey Sharp <ja...@minilop.net> > --- > dix/window.c | 74 +++++++++++++++++++++++++++------------------------------ > 1 files changed, 35 insertions(+), 39 deletions(-) > > diff --git a/dix/window.c b/dix/window.c > index 44bfa18..c87020d 100644 > --- a/dix/window.c > +++ b/dix/window.c > @@ -152,6 +152,12 @@ static unsigned char _back_msb[4] = {0x11, 0x44, 0x22, > 0x88}; > static Bool WindowParentHasDeviceCursor(WindowPtr pWin, > DeviceIntPtr pDev, > CursorPtr pCurs); > +static WindowOptPtr > +AllocateWindowOptional(CursorPtr cursor, > + VisualID visual, > + Colormap colormap, > + Mask dontPropagateMask); > + > static Bool > WindowSeekDeviceCursor(WindowPtr pWin, > DeviceIntPtr pDev, > @@ -481,25 +487,11 @@ CreateRootWindow(ScreenPtr pScreen) > pWin->parent = NullWindow; > SetWindowToDefaults(pWin); > > - pWin->optional = malloc(sizeof (WindowOptRec)); > + pWin->optional = AllocateWindowOptional(None, > + pScreen->rootVisual, pScreen->defColormap, 0); > if (!pWin->optional) > return FALSE; > > - pWin->optional->dontPropagateMask = 0; > - pWin->optional->otherEventMasks = 0; > - pWin->optional->otherClients = NULL; > - pWin->optional->passiveGrabs = NULL; > - pWin->optional->userProps = NULL; > - pWin->optional->backingBitPlanes = ~0L; > - pWin->optional->backingPixel = 0; > - pWin->optional->boundingShape = NULL; > - pWin->optional->clipShape = NULL; > - pWin->optional->inputShape = NULL; > - pWin->optional->inputMasks = NULL; > - pWin->optional->deviceCursors = NULL; > - pWin->optional->colormap = pScreen->defColormap; > - pWin->optional->visual = pScreen->rootVisual; > - > pWin->nextSib = NullWindow; > > pWin->drawable.id = FakeClientID(0); > @@ -3500,18 +3492,21 @@ CheckWindowOptionalNeed (WindowPtr w) > * values. > */ > > -Bool > -MakeWindowOptional (WindowPtr pWin) > +static WindowOptPtr > +AllocateWindowOptional (CursorPtr cursor, > + VisualID visual, > + Colormap colormap, > + Mask dontPropagateMask) > { > - WindowOptPtr optional; > - WindowOptPtr parentOptional; > - > - if (pWin->optional) > - return TRUE; > - optional = malloc(sizeof (WindowOptRec)); > + WindowOptPtr optional = malloc(sizeof (WindowOptRec)); > if (!optional) > - return FALSE; > - optional->dontPropagateMask = DontPropagateMasks[pWin->dontPropagate]; > + return NULL; > + optional->cursor = cursor; > + if (cursor) > + cursor->refcnt++; > + optional->visual = visual; > + optional->colormap = colormap; > + optional->dontPropagateMask = dontPropagateMask; > optional->otherEventMasks = 0; > optional->otherClients = NULL; > optional->passiveGrabs = NULL; > @@ -3523,21 +3518,22 @@ MakeWindowOptional (WindowPtr pWin) > optional->inputShape = NULL; > optional->inputMasks = NULL; > optional->deviceCursors = NULL; > + return optional; > +} >
hi, maybe it is better to use calloc() here to make sure everything is initialised ? I have no clue what you are actually changing but i notice that this is missing: pWin->optional->backingBitPlanes = ~0L; and if you use "ScreenPtr pScreen" instead of "VisualID visual","Colormap colormap" that would offer you more flexibility in the future. btw; - optional->dontPropagateMask = DontPropagateMasks[pWin->dontPropagate]; + optional->dontPropagateMask = dontPropagateMask; it seems that it was set to 0 after that - pWin->optional->dontPropagateMask = 0; but is that intended ? re, wh > +Bool > +MakeWindowOptional (WindowPtr pWin) > +{ > + WindowOptPtr parentOptional; > + > + if (pWin->optional) > + return TRUE; > parentOptional = FindWindowWithOptional(pWin)->optional; > - optional->visual = parentOptional->visual; > - if (!pWin->cursorIsNone) > - { > - optional->cursor = parentOptional->cursor; > - optional->cursor->refcnt++; > - } > - else > - { > - optional->cursor = None; > - } > - optional->colormap = parentOptional->colormap; > - pWin->optional = optional; > - return TRUE; > + pWin->optional = AllocateWindowOptional( > + pWin->cursorIsNone ? None : parentOptional->cursor, > + parentOptional->visual, parentOptional->colormap, > + DontPropagateMasks[pWin->dontPropagate]); > + return pWin->optional != NULL; > } > > /* _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel