Bug 53242

Summary: xwininfo segfaults on invalid screen
Product: xorg Reporter: david.venz
Component: App/xwininfoAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: david.venz, xcb
Version: 7.6 (2010.12)Keywords: patch
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
See Also: https://launchpad.net/bugs/1028274
https://bugzilla.redhat.com/show_bug.cgi?id=808561
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Workaround - Patch against Ubuntu 12.04 source (7.6)
none
0001-Return-connection-failure-if-display-string-specifie.patch
none
libxcb patch, introducing XCB_CONN_CLOSED_INVALID_SCREEN
none
Yet another revision of libxcb patch none

Description david.venz 2012-08-08 12:26:42 UTC 
Created attachment 65279 [details]
Workaround - Patch against Ubuntu 12.04 source (7.6)

This is https://bugs.launchpad.net/ubuntu/+source/x11-utils/+bug/1028274 and https://bugzilla.redhat.com/show_bug.cgi?id=808561

[Impact]
xwininfo segfaults if called with an out-of-range screen number.

[Test Case]
1. xwininfo -root -display :0.0
2. xwininfo -root -display :0.1

Actual Behavior:
 * First command passes
 * Second command segfaults
Expected Behavior:
 * First command passes
 * Second command returns an error message without segfaulting

---------------

On Ubuntu 8.04 this error message used to be "cannot open display :0.1".

The problem could lie in libxcb (I'm no expert) xcb_connection_has_error() no longer returning true in the -display :0.1 case, but I've attached a patch for xwininfo anyway (sorry about the downstream format etc, not ready to jump into git).  My lingering concern is over how many other bits of code use libxcb in this way, and may have had their assumptions broken.  I.e. is the "right" place to fix this problem longer term in libxcb?
Comment 1 Jeremy Huddleston Sequoia 2012-08-25 19:12:29 UTC 
With current master (5037f79e8f6a36d3c524a2dd8150cf96c31b7106):

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000001
0x00000001000016e1 in Select_Window (dpy=0x10081bc00, screen=0x100822bd4, descend=1) at dsimple.c:180
180	    if (grab_reply->status != XCB_GRAB_STATUS_SUCCESS)
(gdb) bt
#0  0x00000001000016e1 in Select_Window (dpy=0x10081bc00, screen=0x100822bd4, descend=1) at dsimple.c:180
#1  0x00000001000031bf in main (argc=4, argv=0x7fff5fbff950) at xwininfo.c:573
Current language:  auto; currently minimal
(gdb) print grab_reply
$1 = (xcb_grab_pointer_reply_t *) 0x0
(gdb)
Comment 2 Alan Coopersmith 2012-08-25 19:16:20 UTC 
Took the question of where to check, libxcb or callers like xwininfo,
to the xcb mailing list for discussion:
http://lists.freedesktop.org/archives/xcb/2012-August/007840.html
Comment 3 Jeremy Huddleston Sequoia 2012-08-25 19:37:25 UTC 
Created attachment 66113 [details] [review]
0001-Return-connection-failure-if-display-string-specifie.patch

Here's a patch for libxcb based on alanc's libxcb patch, but it introduces a new error code to allow callers to tell the difference between the two errors.
Comment 4 Jeremy Huddleston Sequoia 2012-08-25 19:50:49 UTC 
Created attachment 66114 [details] [review]
libxcb patch, introducing XCB_CONN_CLOSED_INVALID_SCREEN

Addresses a couple issues in the original patch
Comment 5 Alan Coopersmith 2012-08-25 19:59:06 UTC 
Created attachment 66115 [details] [review]
Yet another revision of libxcb patch

Merged my fixes with Jeremy's improvements to the previous libxcb patches.
Yay for remote pair programming via bugzilla/irc!  (Sorry for the rapid
fire noise for everyone else.)
Comment 6 Alan Coopersmith 2012-08-30 05:05:26 UTC 
Fixes pushed to git master for next releases:

To ssh://git.freedesktop.org/git/xorg/app/xwininfo
   53564df..aedc2ec  master -> master

To ssh://git.freedesktop.org/git/xcb/libxcb
   ed93a6a..ff53285  master -> master

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.