[PATCH 2/3] Factor out duplicated WindowOptPtr initialization.

[walter harms]

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 <jamey at 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;
>  }
>  
>  /*