[PATCH] xfree86: belately init RandR12 if xinerama fails. (#24627)

[Peter Hutterer]

On Tue, Dec 08, 2009 at 06:00:38PM -0800, Aaron Plattner wrote:
> On Tue, Dec 08, 2009 at 05:55:14PM -0800, Peter Hutterer wrote:
> > On Tue, Dec 08, 2009 at 05:24:06PM -0800, Aaron Plattner wrote:
> > > On Tue, Dec 08, 2009 at 03:52:27PM -0800, Peter Hutterer wrote:
> > > > Xorg +xinerama crashes immediately due to whacky dependency between Xinerama
> > > > and RandR12. The latter doesn't initialize if Xinerama is enabled, but if
> > > > only one screen is found, Xinerama is disabled again and RandR12 tries to
> > > > access data it never initialized.
> > > > 
> > > > Dependency chain is:
> > > > - ProcessCommandLine sets noPanoramiXExtension to FALSE
> > > > - xf86RandR12Init() is a noop
> > > > - PanoramiXExtensionInit sets noPanoramiXExtension to TRUE
> > > > - xf86RandR12CreateScreenResources tries to use the devPrivates key it never
> > > >   initialized.
> > > > 
> > > > This hack checks if the key was initialized if noPanoramiXExtension is TRUE.
> > > > If not, re-initialize RandR12, this time for reals.
> > > > 
> > > > X.Org Bug 24627 <http://bugs.freedesktop.org/show_bug.cgi?id=24627>
> > > > 
> > > > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > > > ---
> > > > 
> > > > Don't know if there are any side-effects of doing this.
> > > > 
> > > >  hw/xfree86/modes/xf86RandR12.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
> > > > index 6ea9d26..dd80ee0 100644
> > > > --- a/hw/xfree86/modes/xf86RandR12.c
> > > > +++ b/hw/xfree86/modes/xf86RandR12.c
> > > > @@ -767,6 +767,8 @@ xf86RandR12CreateScreenResources (ScreenPtr pScreen)
> > > >      /* XXX disable RandR when using Xinerama */
> > > >      if (!noPanoramiXExtension)
> > > >  	return TRUE;
> > > > +    else if (!xf86RandR12Key)
> > > > +        xf86RandR12Init(pScreen);
> > > 
> > > This makes me nervous because it starts calling a big chain of
> > > ScreenInit-style functions at CreateScreenResources time.  I can't find any
> > > concrete examples of things that would fail, but it perturbs the init order
> > > pretty drastically, but only in obscure Xinerama cases.  Would it be
> > > acceptable to just bail out without doing anything if (!xf86RandR12Key)?
> > 
> > that'd be the following patch then which - afaict - works too. Though the
> > same predicament stands, I don't know if there are any side-effects. Keith,
> > any comments?
> > 
> > From ca7aa60a580e8c15016b05b75c62f2c45eee01b7 Mon Sep 17 00:00:00 2001
> > From: Peter Hutterer <peter.hutterer at who-t.net>
> > Date: Wed, 9 Dec 2009 09:45:39 +1000
> > Subject: [PATCH] xfree86: don't set up RandR12 screen resources not initialized. (#24627)
> > 
> > Xorg +xinerama crashes immediately due to whacky dependency between Xinerama
> > and RandR12. The latter doesn't initialize if Xinerama is enabled, but if
> > only one screen is found, Xinerama is disabled again and RandR12 tries to
> > access data it never initialized.
> > 
> > Dependency chain is:
> > - ProcessCommandLine sets noPanoramiXExtension to FALSE
> > - xf86RandR12Init() is a noop
> > - PanoramiXExtensionInit sets noPanoramiXExtension to TRUE
> > - xf86RandR12CreateScreenResources tries to use the devPrivates key it never
> >   initialized.
> > 
> > If we didn't initialize RandR12, don't try to set anything up either.
> > 
> > X.Org Bug 24627 <http://bugs.freedesktop.org/show_bug.cgi?id=24627>
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  hw/xfree86/modes/xf86RandR12.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/xfree86/modes/xf86RandR12.c b/hw/xfree86/modes/xf86RandR12.c
> > index 6ea9d26..1a4f2bd 100644
> > --- a/hw/xfree86/modes/xf86RandR12.c
> > +++ b/hw/xfree86/modes/xf86RandR12.c
> > @@ -765,7 +765,7 @@ xf86RandR12CreateScreenResources (ScreenPtr pScreen)
> >      int			mmWidth, mmHeight;
> >  #ifdef PANORAMIX
> >      /* XXX disable RandR when using Xinerama */
> > -    if (!noPanoramiXExtension)
> > +    if (!noPanoramiXExtension || !xf86RandR12Key)
> >  	return TRUE;
> >  #endif
> 
> I'd recommend moving the !xf86RandR12Key case outside the #ifdef to catch
> any other cases where it failed to initialize, but otherwise,
> Acked-by: Aaron Plattner <aplattner at nvidia.com>

Arguably, not having the key defined in any other situation seems like a bug
that needs fixing - leaving the crash there might just be better as a big
cluebat.

it's just this special case that seems a valid configuration yet leads to a
segfault.

Cheers,
  Peter