Bug 4248 - rdesktop crashes the server when Composite extension is enabled
Summary: rdesktop crashes the server when Composite extension is enabled
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: Other Linux (All)
: high normal
Assignee: Benjamin Herrenschmidt
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-25 19:54 UTC by Benjamin Herrenschmidt
Modified: 2005-09-04 07:42 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix crash on cmap alloc failure and fix failure with 32 bits visual (2.80 KB, patch)
2005-09-04 21:21 UTC, Benjamin Herrenschmidt
no flags Details | Splinter Review
Fix colormap private allocation in dix (1.25 KB, patch)
2005-09-04 23:47 UTC, Benjamin Herrenschmidt
no flags Details | Splinter Review
Fix colormap on 32 bits visuals (3.20 KB, patch)
2005-09-04 23:48 UTC, Benjamin Herrenschmidt
no flags Details | Splinter Review

Description Benjamin Herrenschmidt 2005-08-25 19:54:43 UTC
When Composite extension is enabled in the config file, launching rdesktop
causes an immediate crash of the server. I've tried using gdb to trace it but
gdb kept blowing up in all sort of funny ways and I didn't go very far. I traced
the protocol exchanges though and there is not much happening before the server
dies:

 - CreateGC,QueryExtension(BIG-REQUESTS),GetProperty (proper reply)
 - BIG-REQUESTS (opcode 131) (proper reply)
 - QueryExtensions(XKEYBOARD) (proper reply)
 - XKEYBOARD (opcode 149) (proper reply)
 - CreateColorMap,GetModifierMapping  --> Server dies

The details of CreateColorMap are:

opcode 78, alloc 0, request-length 4, mid 0x00400001, window 0x00000045,
visualid 0x00000043

The details of GetModifierMapping are:

opcode 119, unused byte, request-length 1

I suspect it dies in CreateColorMap. I've tried to trace it with gdb, It went to
LbxCreateColorMap() which did an indirect pointer call which ended up into
CMapCreateColorMap() which did an indirect pointer call that I didn't manage to
trace (gdb screws up everytime). It does however go back out both functions, so
it's not dying in there.

It's also not getting to any GetModifierMapping that I could find though.

I've added a breakpoint to dix/colormap.c at the exit of the pointer call that
led to LbxCreateColorMap, and it _seems_ we are getting to the FreeResource(mid,
RT_NONE) case (the above call returned 0) and we die in there (we never reach
return BadAlloc;) but it could be some gdb confusion
Comment 1 Benjamin Herrenschmidt 2005-08-31 01:06:45 UTC
As per Keith and Daniel suggestions, I confirm that running Keith' transparent
xterm or fdclock -t both kill the server right away. So it must have something
to do with using that ARGB visual.
Comment 2 Benjamin Herrenschmidt 2005-09-04 21:21:34 UTC
Created attachment 3170 [details] [review]
Fix crash on cmap alloc failure and fix failure with 32 bits visual

This patch fixes the bug, which was in fact two bugs. Here's a copy of the
explanation I posted to the xorg mailing list:
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 ?)
Comment 3 Keith Packard 2005-09-04 23:45:29 UTC
As discussed on IRC, X supports colormaps with up to 16 bit components, so it
seems reasonable to limit colormaps to that same depth; a function which
explicitly checks depth against 16 and only then goes on to compute 1 << depth
should work fine.
Comment 4 Benjamin Herrenschmidt 2005-09-04 23:47:27 UTC
Created attachment 3171 [details] [review]
Fix colormap private allocation in dix

Ok, as per discussion with Keith on IRC, I broke up the patch in 2. This one
fixes the allocation of colormaps devPrivate array so it's properly NULL'ified
Comment 5 Benjamin Herrenschmidt 2005-09-04 23:48:43 UTC
Created attachment 3172 [details] [review]
Fix colormap on 32 bits visuals

And this patch fixes the colormap code for 32 bits visuals by separating the
test into a function that only does 1 << nplanes when nplanes <= 16
Comment 6 Daniel Stone 2005-09-05 00:42:53 UTC
both patches committed to HEAD


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.