Opened 13 years ago

Closed 12 years ago

#3401 closed defect (fixed)

mythtv-setup crashes when using remote X display (via ssh)

Reported by: anonymous Owned by: danielk
Priority: minor Milestone: 0.20
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

I am trying to install the backend on a machine that has no local X server. The display is being forwarded to a MacOSX machine via SSH (-X -Y). This might be a bug in libxrandr.

Starting program: /home/gal/src/mythtv/mythtv/programs/mythtv-setup/mythtv-setup [Thread debugging using libthread_db enabled] [New Thread -1245915440 (LWP 21268)] Qt: gdb: -nograb added to command-line options.

Use the -dograb option to enforce grabbing.

Xlib: extension "XInputExtension" missing on display "localhost:10.0". Failed to get list of devices 2007-05-02 18:50:18.967 Using runtime prefix = /usr 2007-05-02 18:50:18.984 DPMS is not supported. 2007-05-02 18:50:19.280 New DB connection, total: 1 2007-05-02 18:50:19.305 Connected to database 'mythconverg' at host: localhost 2007-05-02 18:50:19.309 Total desktop dim: 1280x800, with 1 screen[s]. 2007-05-02 18:50:19.348 Using screen 0, 1280x778 at 0,22 2007-05-02 18:50:19.375 Current Schema Version: 1188

Program received signal SIGSEGV, Segmentation fault. [Switching to Thread -1245915440 (LWP 21268)] 0xb69a6454 in ?? () from /usr/lib/libXrandr.so.2 (gdb) bt #0 0xb69a6454 in ?? () from /usr/lib/libXrandr.so.2 #1 0x000052e9 in ?? () #2 0x08148d18 in ?? () #3 0x00000048 in ?? () #4 0xb608bb2c in ?? () from /usr/lib/libX11.so.6 #5 0x00000000 in ?? () (gdb) quit

Attachments (1)

noxrandr.1.patch (1.5 KB) - added by Dibblah 13 years ago.
This patch is a similar fix. Fixes in the case where XRandR has been configured, but is no longer available as well.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 13 years ago by gal

The cause is XRR not being checked before calling into it. The attached patch fixes the problem for me.

Index: DisplayResX.cpp
===================================================================
--- DisplayResX.cpp     (revision 13397)
+++ DisplayResX.cpp     (working copy)
@@ -121,7 +121,10 @@
 
     X11L;
     Window root = RootWindow(display, DefaultScreen(display));
-    XRRScreenConfiguration *cfg = XRRGetScreenInfo(display, root);
+    XRRScreenConfiguration *cfg = NULL;
+    int event_base, error_base;
+    if (XRRQueryExtension(display, &event_base, &error_base))
+       cfg =  XRRGetScreenInfo(display, root);
     if (!cfg)
     {
         if (display)

comment:2 Changed 13 years ago by tdegibson_at_gmail_dot_com

Also seg fault from running mythtv-setup via vnc window... downgrading to libXrandr-1.1.1 from libXrandr-1.2.1 on Gentoo fixed seg fault but obviously not an ideal fix...

Tim

Changed 13 years ago by Dibblah

Attachment: noxrandr.1.patch added

This patch is a similar fix. Fixes in the case where XRandR has been configured, but is no longer available as well.

comment:3 Changed 13 years ago by danielk

Owner: changed from Isaac Richards to danielk

comment:4 Changed 12 years ago by eloy@…

The patch by Dibblah is bogus since it calls XRRQueryExtension() with two NULL pointers. If you look at the source for XRRQueryExtension() you'll see that the function provides return values via these two pointers. If the pointers are NULL then we have a nice crash, like what we're currently having in the 0.20-svn20070523-0.0 Debian packages maintained by Christian Marillat since he integrated this patch.

comment:5 Changed 12 years ago by anonymous

eloy: You're completely wrong. XRRQueryExtension() can and should be called with two NULL pointers. You can first start by reading the man page, http://www.die.net/doc/linux/man/man3/xrrqueryextension.3.html

Then read the code. Those are called "out variables" or callback variables. If you don't care about the data coming to you, you set them equal to NULL and all is well.

comment:6 in reply to:  5 Changed 12 years ago by eloy@…

Replying to anonymous:

eloy: You're completely wrong. XRRQueryExtension() can and should be called with two NULL pointers. You can first start by reading the man page, http://www.die.net/doc/linux/man/man3/xrrqueryextension.3.html

Then read the code. Those are called "out variables" or callback variables. If you don't care about the data coming to you, you set them equal to NULL and all is well.

anonymous: if you set to NULL the pointers to your "out variables" in the specific case of XRRQueryExtension() then nothing is well.

I read the code before claiming that the patch is bogus. It seems like you haven't read it, or if you read it you didn't understand the problem, so how about we read the code together? See:

http://csourcesearch.net/package/kdrive/4.3.0/xc/lib/Xrandr/Xrandr.c

XRRQueryExtension(Display *dpy, int *event_basep, int *error_basep) is defined at line 339.

If the function parameters event_basep and error_basep are NULL then lines 344 and 345 will be writing something to address 0x00000000. I am sure you know that doing this is bad, and will cause a crash.

Trust me on this one - the patch is bogus; you can't pass NULL in the two pointers that XRRQueryExtension() receives.

This patch was applied to the MythTV Debian packages and had to be backed out because the backend was crashing as soon as it started. Debugging showed that it was crashing because of the problem that I describe above.

The correct way to call XRRQueryExtension() is to pass real, valid pointers, not NULL pointers.

comment:7 Changed 12 years ago by danielk

Resolution: fixed
Status: newclosed

(In [13766]) Fixes #3401. Applies slightly modified version of patch.

This just checks if we have the xrandr extension before using it. This fixes a segfault for people recompile their own X11 servers to exclude XRandR support after compiling MythTV, or when they have X11 installed on an OSX machine.

I added the dummy variables argued over in the ticket comments. While I don't think they are required by the API, an implementation of XRandR in wide release appears to require them in practice, whether this is a new bug in the implementation or a new bug in the API isn't really important; the code that works with this XRandR release is still correct according API and only one line longer.

comment:8 Changed 12 years ago by anonymous

Will this get backported to -fixes?

comment:9 Changed 12 years ago by stuartm

Milestone: unknown0.20
Resolution: fixed
Status: closedreopened

This needs backporting to -fixes, unless there are any objections I'll do it?

comment:10 Changed 12 years ago by dan@…

Thanks Stuart!

comment:11 Changed 12 years ago by stuartm

Resolution: fixed
Status: reopenedclosed

(In [14223]) Backports [13766] to -fixes.

Fixes a segfault with mythtv-setup when using X11 forwarding and XRandR is not available.

See #3401 for more detail.

Fixes #3401

Note: See TracTickets for help on using tickets.