Bug 1478

Summary: Selection.c damages user error handler function
Product: xorg Reporter: Michael <mikets>
Component: Lib/XtAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: critical    
Priority: highest CC: phz
Version: 6.8.1Keywords: patch
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
src/Selection.c
none
Diff of HandleSelectionEvents (base is libXt-1.0.4) none

Description Michael 2004-09-27 03:53:53 UTC 
It looks like under certain conditions a user error handler routine set by 
XSetErrorHandler can be effectively canceled from inside of Xt/Selection.c.

The place to suspect is the loop in the SelectionRequest case of 
HandleSelectionEvents function. Under certain conditions there can be several 
two calls to EndProtectedSection after single StartProtectedSection. Since 
EndProtectedSection zeroes the global oldErrorHandler variable, the second 
call to EndProtectedSection restores the default error procedure instead of 
user's one.

Probably there exist more than one problematic code fragments.
(Checked in 6.6 and 6.8.1 sources).
Comment 1 Josh Triplett 2005-05-05 00:06:39 UTC 
*** Bug 1479 has been marked as a duplicate of this bug. ***
Comment 2 Daniel Stone 2007-02-27 01:24:16 UTC 
Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.
Comment 3 Ryan Hajdaj 2008-05-22 20:21:23 UTC 
Created attachment 16700 [details]
src/Selection.c

HandleSelectionEvents() (SelectionRequest case) has been modified for balanced "EndProtectedSectioning".

Note: This "patch" is based on libXt-1.0.4 from the releases section.
Comment 4 Ryan Hajdaj 2008-05-23 07:05:06 UTC 
Created attachment 16706 [details]
Diff of HandleSelectionEvents (base is libXt-1.0.4)

Here's the diff of the change (again, based on libXt-1.0.4).
Note: I was not able to find a problem with the loop logic, namely since whenever GetConversion returns true, it has always called StartProtectedSection.

The potential fix here is to call StartProtectedSection in the non-for-loop if so that there is again a balance of Start/StopProtectedSection calls.
Comment 5 Pete Zakel 2010-06-23 17:10:45 UTC 
We have discovered we are having numerous crashes in our software due to this bug.  We have implemented a sledgehammer approach of continually resetting the X error handler in our event loop, but having this fix actually made to the code would solve the crashes without the workaround.

Is there any possibility that this fix can actually be made to the released code base?
Comment 6 Alan Coopersmith 2010-09-23 18:16:12 UTC 
Sorry about the delay - libXt is mostly unmaintained these days since so
little uses it and no one involved knows much about it.   (Of course, should
anyone who knows much about it and needs it to work for their software want
to join X.Org to help maintain this library, we're always looking for more
volunteers.)

I can't claim to know enough about this fix to review it, but it seems 
simple enough that I've gone ahead and pushed it to git master anyway 
so others can start testing. 

Patches are generally handled most quickly when submitted as actual 
context diffs, not replacement source files or HTML dumps - see
http://www.x.org/wiki/Development/Documentation/SubmittingPatches
for guidelines.

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.