[BUG] xf86cmap.c and 32 bits ARGB visual

[Benjamin Herrenschmidt]

Hi !

I found why using the 32 bits ARGB visual cause an immediate server
crash on ppc. In fact, the bug is not ppc specific, but we trigger some
undefined result at some point. There are actually 2 different bugs
going on:

 1) The actual cause of the crash is a problem with failure handling in
CMapCreateColormap(). It doesn't initialize the cmap private ptr to NULL
first, thus if any failure happens when calling down the chain or
calling CMapAllocateColormapPrivate(), we will have a dangling value in
there. Later on, due to the failure, CMapDestroyColormap() will be
called, which will try to use that dangling value and crash. The fix is
to initialize the private pointer to NULL:


 static Bool 
 CMapCreateColormap (ColormapPtr pmap)
 {
     ScreenPtr pScreen = pmap->pScreen;
     CMapScreenPtr pScreenPriv = 
         (CMapScreenPtr)pScreen->devPrivates[CMapScreenIndex].ptr;
     Bool ret = FALSE;

     pScreen->CreateColormap = pScreenPriv->CreateColormap;
+    pmap->devPrivates[CMapColormapIndex].ptr = NULL;
     if((*pScreen->CreateColormap)(pmap)) { 
 	 if(CMapAllocateColormapPrivate(pmap)) 
	    ret = TRUE;
     }
     pScreen->CreateColormap = CMapCreateColormap;

     return ret;
 }


 2) The above turns the crash into a simple failure of the application.
Now the reason why it fails is because CMapAllocateColormapPrivate() fails.
The reason why this later fails is because it uses a construct (which is
used all over the xf86cmap.c code) that looks like:

    if((1 << pmap->pVisual->nplanes) > pScreenPriv->maxColors)
	numColors = pmap->pVisual->ColormapEntries;
    else 
	numColors = 1ull << pmap->pVisual->nplanes; 

In our case, pmap->pVisual->nplanes is 32, thus (1 << pmap->pVisual->nplanes)
is undefined, especially on a 32 bits architecture. This cause the subsequent
xalloc() to do random things based on what we have here. On ppc, we are lucky,
it gets a nice 0 and we fail right away. In other circumstances, you might end
up allocating random amount of memory...

This construct is used in many places in xf86cmap.c to calulate the size of
the color map to use.

The "simple" way to fix it, as per the patch below, is to turn 1 into 1ull
(unsigned long long) thus causing the test to properly detect that a 32
planes visual has too many colors. 

If use of long longs is a problem for X, then we should replace that construct
with a function that explicitely test if nplanes is too big for the native
integer size (or simply > 32, do we support platforms where unsigned int is
not 32 bits ?)

Here's the patch. If accepted, it should close bug #4248

diff -urN xc-HEAD.orig/programs/Xserver/hw/xfree86/common/xf86cmap.c xc-HEAD/programs/Xserver/hw/xfree86/common/xf86cmap.c
--- xc-HEAD.orig/programs/Xserver/hw/xfree86/common/xf86cmap.c	2005-08-17 11:55:35.000000000 +1000
+++ xc-HEAD/programs/Xserver/hw/xfree86/common/xf86cmap.c	2005-09-05 14:14:52.000000000 +1000
@@ -255,10 +255,10 @@
     int numColors;
     LOCO *colors;
 
-    if((1 << pmap->pVisual->nplanes) > pScreenPriv->maxColors)
+    if((1ull << pmap->pVisual->nplanes) > pScreenPriv->maxColors)
 	numColors = pmap->pVisual->ColormapEntries;
     else 
-	numColors = 1 << pmap->pVisual->nplanes; 
+	numColors = 1ull << pmap->pVisual->nplanes; 
 
     if(!(colors = xalloc(numColors * sizeof(LOCO))))
 	return FALSE;
@@ -295,6 +295,7 @@
     Bool ret = FALSE;
 
     pScreen->CreateColormap = pScreenPriv->CreateColormap;
+    pmap->devPrivates[CMapColormapIndex].ptr = NULL;
     if((*pScreen->CreateColormap)(pmap)) { 
 	if(CMapAllocateColormapPrivate(pmap)) 
 	   ret = TRUE;
@@ -371,7 +372,7 @@
 	   (CMapColormapPtr) pmap->devPrivates[CMapColormapIndex].ptr;
 	int i;
 
-        if((1 << pVisual->nplanes) > pScreenPriv->maxColors) {
+        if((1ull << pVisual->nplanes) > pScreenPriv->maxColors) {
 	    int index;
 
 	    num = 0;
@@ -442,7 +443,7 @@
 
     if(!(pScreenPriv->flags & CMAP_PALETTED_TRUECOLOR) &&
 	(pmap->pVisual->class == TrueColor) &&
-	((1 << pmap->pVisual->nplanes) > pScreenPriv->maxColors))
+	((1ull << pmap->pVisual->nplanes) > pScreenPriv->maxColors))
 		return;
 
     if(LOAD_PALETTE(pmap, index))
@@ -572,7 +573,7 @@
 	}
 	break;
     case TrueColor:
-	if((1 << pVisual->nplanes) > pScreenPriv->maxColors) {
+	if((1ull << pVisual->nplanes) > pScreenPriv->maxColors) {
 	    for(i = 0; i <= reds; i++) 
 		colors[i].red   = gamma[i * maxValue / reds].red;
 	    for(i = 0; i <= greens; i++) 
@@ -615,7 +616,7 @@
 	}
 	break;
     case DirectColor:
-	if((1 << pVisual->nplanes) > pScreenPriv->maxColors) {
+	if((1ull << pVisual->nplanes) > pScreenPriv->maxColors) {
 	    for(i = 0; i < defs; i++) { 
 		index = indices[i];
 		if(index <= reds)
@@ -932,7 +933,7 @@
 
 	if(!(pScreenPriv->flags & CMAP_PALETTED_TRUECOLOR) &&
 	    (pMap->pVisual->class == TrueColor) &&
-	    ((1 << pMap->pVisual->nplanes) > pScreenPriv->maxColors)) {
+	    ((1ull << pMap->pVisual->nplanes) > pScreenPriv->maxColors)) {
 
 	    /* if the current map doesn't have a palette look
 		for another map to change the gamma on. */
@@ -1019,7 +1020,7 @@
 
         if(!(pScreenPriv->flags & CMAP_PALETTED_TRUECOLOR) &&
             (pMap->pVisual->class == TrueColor) &&
-            ((1 << pMap->pVisual->nplanes) > pScreenPriv->maxColors)) {
+            ((1ull << pMap->pVisual->nplanes) > pScreenPriv->maxColors)) {
 
             /* if the current map doesn't have a palette look
                 for another map to change the gamma on. */