Tk Source Code

By accident, I stumbled across a sequence of Tk statements causing a segfault on Linux, with KDE wm:

--
package require Tk
toplevel .t
wm deiconify .t
tkwait visibility .t
grab .
after idle {
	event generate .t <ButtonPress-1>
	event generate .t <ButtonRelease>
}
--

Admitted, it's a weird sequence (with the intervening [grab]) and I can't imagine a useful use-case for it. It ought not segfault though.

The crash occurs almost identically in the following cases:
a. latest trunk (tcl: 12da6c8acd and tk: de0c994c).
b. with TIP #532 adaptations from branch bug6e8afe516d-87 (tk: 057fd0ff with released tcl-8.6.8)
 
A full back trace for case a. is attached to this ticket as a separate file. The concise back trace is appended below.

Regards,
Erik Leunissen

-- concise back trace (case a.) --
#0  0x00007fec73d54dcd in TkPointerEvent (eventPtr=0x7fff08be5a60, winPtr=0x2620430)
    at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkGrab.c:885
#1  0x00007fec73d47663 in InvokeMouseHandlers (winPtr=0x2620430, mask=8, eventPtr=0x7fff08be5a60)
    at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkEvent.c:299
#2  0x00007fec73d484da in Tk_HandleEvent (eventPtr=0x7fff08be5a60) at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkEvent.c:1277
#3  0x00007fec73d362d3 in HandleEventGenerate (interp=0x2451680, mainWin=0x24bc200, objc=2, objv=0x245cd50)
    at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkBind.c:3471
#4  0x00007fec73d34341 in Tk_EventObjCmd (clientData=0x24bc200, interp=0x2451680, objc=4, objv=0x245cd40)
    at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkBind.c:2426
#5  0x00007fec738e9933 in Dispatch (data=0x26368d8, interp=0x2451680, result=0) at /usr/local/src/SOURCES/tcl-trunk/generic/tclBasic.c:4313
#6  0x00007fec738e999a in TclNRRunCallbacks (interp=0x2451680, result=0, rootPtr=0x0)
    at /usr/local/src/SOURCES/tcl-trunk/generic/tclBasic.c:4329
#7  0x00007fec738ec2e4 in TclEvalObjEx (interp=0x2451680, objPtr=0x2620430, flags=131072, invoker=0x0, word=0)
    at /usr/local/src/SOURCES/tcl-trunk/generic/tclBasic.c:5772
#8  0x00007fec738ec27a in Tcl_EvalObjEx (interp=0x2451680, objPtr=0x24bc200, flags=131072)
    at /usr/local/src/SOURCES/tcl-trunk/generic/tclBasic.c:5753
#9  0x00007fec73a4d32e in AfterProc (clientData=0x2614380) at /usr/local/src/SOURCES/tcl-trunk/generic/tclTimer.c:1181
#10 0x00007fec73a4c443 in TclServiceIdle () at /usr/local/src/SOURCES/tcl-trunk/generic/tclTimer.c:751
#11 0x00007fec73a1e302 in Tcl_DoOneEvent (flags=-3) at /usr/local/src/SOURCES/tcl-trunk/generic/tclNotify.c:980
#12 0x00007fec73d49659 in Tk_MainLoop () at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkEvent.c:2148
#13 0x00007fec73d5dd9f in Tk_MainEx (argc=-1, argv=0x7fff08be6198, appInitProc=0x400abf <Tcl_AppInit>, interp=0x2451680)
    at /usr/local/src/SOURCES/tk-trunk/unix/../generic/tkMain.c:395
#14 0x0000000000400ab8 in main (argc=2, argv=0x7fff08be6188) at /usr/local/src/SOURCES/tk-trunk/unix/../unix/tkAppInit.c:81
--

Merged to core-8-6-branch and trunk.

Ah, I see my misunderstanding.

I misread the "!=".

Apologies!

Hmm,

Are we talking about the same things? I see in the latest commit:

if (eventPtr->xbutton.button != AnyButton &&
		    ((eventPtr->xbutton.state & ALL_BUTTONS)
		    == buttonStates[eventPtr->xbutton.button - Button1])) 

It's my understanding that the part before "&&" in this condition, as well as the part after "&&" will have to be evaluated in order to obtain the evaluation result for the entire condition.

If that's wrong, I apologize and will seek re-education (which I don't expect from this bug tracker).

Both commit [abb5ea6063] and commit [ec435697] prevent the crash because both are testing the specific case <ButtonRelease> (i.e. eventPtr->xbutton.button == AnyButton) first, before the second the condition is possibly evaluated. This second condition is not evaluated when the 'if' result is determined by the first condition alone. That's exactly why the fix prevents the crash.

> you did change the condition (with commit ec435697) such that
> the second part is also evaluated when the event is <ButtonRelease>

Incorrect. When the event is <ButtonRelease>, with both commits the second part of the condition is not evaluated. This results from careful reading of the code, moreover if it was not the case there would still be a crash (and there is none, with either commit).

The difference between these two commits is that, when the event is <ButtonRelease>, my first commit generates the leave/enter events (i.e. calls ReleaseButtonGrab) but my second commit does not. I think the latter is more correct, for the reasons I explained below. Note also that this problem has been present since the beginning of the repository history (1998) without anyone noticing until yesterday, it really is a corner case.

But I see that you did change the condition (with commit ec435697) such that the second part is also evaluated when the event is <ButtonRelease>. That's what corresponds to my fuzzy logic which was at the base of my silly formulated previous comment.

I rebuilt Tk and ran the reproducible script: no crash.

Seems fine to me now.

Yes, I agree.

I was confusing the semantics of C with that of a natural language, which is quite silly. Sorry.

Well, no. OK, let's enter in the details of what this all means (in my understanding at least).

From the X documentation about XButtonEvent:

- eventPtr->xbutton.state indicates the logical state of the pointer buttons and modifier keys just prior to the event.

- eventPtr->xbutton.button represents the pointer button that changed state.

Therefore, in the eventPtr->type == ButtonRelease branch of the if statement, the test on

(eventPtr->xbutton.state & ALL_BUTTONS) == buttonStates[eventPtr->xbutton.button - Button1]

checks whether the button that was just released in the event is the last one to be released (in more details: a single button mask matching the just released button corresponds to the buttons state just before the event).

This of course is working fine only if eventPtr->xbutton.button >= Button1.

But the test script in this ticket generates an event with just <ButtonRelease>, that is it does not specify which button is released. From the documentation, the meaning this conveys is that any button (or an unspecified button) is released. In this case, eventPtr->xbutton.button has value 0, which is not supposed to happen in the above test and is the cause for the crash. I think the original developer only thought of events generated by the OS from a real mouse, i.e. a real button is released, and this button has a non-zero number. Generation of <ButtonRelease> event through a script seems to have been overlooked in this section of the code in tkGrab.c.

I don't see how the OS could generate a <ButtonRelease> event, it should always rather generate a <ButtonRelease-x> event (with x being the number of the actually released physical button). As a consequence, if we encounter a case where eventPtr->xbutton.button == 0 it must mean that the <ButtonRelease> event was generated programmatically. 'AnyButton' is precisely defined to be zero in X.h and this value indeed conveys the right meaning (i.e.: no specific button).

Now what should we do when eventPtr->xbutton.button == AnyButton ? I originally thought we should generate the leave/enter events, which is what I proposed in [abb5ea6063]. Thinking twice, and because this case can only happen from programmatically generated <ButtonRelease> events, I think this case should be ignored. This is now [ec435697].

Agreed?

W.r.t. the condition in the proposed fix:

 (eventPtr->xbutton.button == AnyButton ||
		    ((eventPtr->xbutton.state & ALL_BUTTONS)
		    == buttonStates[eventPtr->xbutton.button - Button1]))

I'm unfamiliar with the exact definition of "AnyButton". If I take that literally, then I expect the first part of the condition to be always true. The condition as a whole would then be moot, and the code code be reduced further by omitting the condition entirely, to:

   883  	} else {
   886  		ReleaseButtonGrab(dispPtr);			/* Note 4. */
   888  	}

I have proposed a fix, see [abb5ea6063].

The crash seems to happen because on this line we have:

eventPtr->xbutton.button = 0
Button1 = 1

Therefore we're apparently accessing array buttonStates before its first element.

Note also that the first event generate is not needed to trigger the crash, the one on <ButtonRelease> is enough.

Finally, specifying the number of the released button (i.e.<ButtonRelease-1>) in the event generate command prevents the crash.