#4525 Unset PATTERN unsafe with trace-originated secondary unsets

[Stephen Huntley]

I wrote a simple application to gather Tk events and pass them from one interpreter to another via tequila (http://wiki.tcl.tk/1243), which uses traces and socket communication to keep array values in sync between Tcl interpreters.

My application displays a canvas, to which a <Motion> event is bound. Thus as the cursor is moved over the canvas a steady stream of Tk events is generated; the scripts associated with these events are captured and written to an array, which tequila echoes to an interpreter in a separate process.

When I start this application and move the cursor continually over the canvas, sometime between five and sixty seconds later one of the interpreters segfaults and crashes.

Platform is Ubuntu Hardy, with ActiveTcl 8.5.5 installed. I duplicated the bug using Pat Thoyt's tclkits versions 8.5.8 (http://www.patthoyts.tk/tclkit/linux-ix86/8.5.8/tclkit%2d8.5.8.gz) and 8.6b1.1 (http://www.patthoyts.tk/tclkit/linux-ix86/8.6%2dbeta/tclkit%2d8.6b1.1.gz).

For ease of duplication I created a starkit containing all relevant code called echoevent.kit (attached). To duplicate:

1. in a terminal console, execute: tclkit echoevent.kit server
This will start a tequila server.

2. in a separate terminal console, execute: tclkit echoevent.kit master
This will start a simple Tk "master" application that displays a canvas. Moving the cursor across this "master" canvas will cause the command associated with the <Motion> event to echo repeatedly to the terminal console.

3. in a third terminal console, execute: tclkit echoevent.kit slave
This will cause an identical "slave" Tk application to appear, except event processing for the "slave" canvas is disabled. Moving the cursor across the "master" canvas will cause the event command to echo to both the "master" console and the "slave" console. The tequila server is passing event information from the master interpreter to the slave interpreter.

4. move the cursor continually over the master canvas to generate a constant stream of <Motion> events to be echoed to the consoles. Either the master or the slave process will crash with a segmentation fault within 60 seconds.

report cross-posted at: http://bugs.activestate.com/show_bug.cgi?id=85927

[Alexandre Ferrieux]

Donal, what you've just committed is not correct yet. The leak is still there.
I notice that your first CleanupVar is dangerous; if it succeeds, varPtr2 is now a freed structure, and yet is used in making decision.
Why not use the CleanupVar logic I posted ?

[Alexandre Ferrieux]

Fixing HEAD ;-)

[Alexandre Ferrieux]

Attaching the fix. Several things to note:
- removed the refcount-- == 1 logic, since it is wrong in that case, and CleanupVar does it too, and does it right
- moved CleanupVar back to the bottom of the loop in the "else" of !IsVarUndefined
- this fixes the leak, avoids using the var after freeing, and makes your comments accurate again ;-)

[Donal K. Fellows]

Your patch is still wrong. ;-)

More importantly, the whole semantics for how you're going about it is wrong too, and no single-iteration strategy can fix it. To show what I mean, here's a sample session (using the HEAD, but apply any fixes you want...)

% trace add variable foo unset {apply {{a b c} {if {$b eq "b"} {global foo; for {set i 0} {$i<100} {incr i} {set foo($i) 1}}}}}
% array set foo {b 1 f 1 j 1}
% parray foo
foo(b) = 1
foo(f) = 1
foo(j) = 1
% array unset foo *
% parray foo
foo(35) = 1
foo(36) = 1
foo(37) = 1

The fundamental issue is that when an unset trace tinkers with the contents of the array, [array unset]'s semantics go haywire. The only thing I can think of to fix this is to use a multi-step deletion process which first calculates which elements to kill and then nukes them while calling traces. (The confusion about what happens when a trace updates an element that is about to be deleted vs one that has already been deleted is probably an indication that we'll just have to be careful.) I suggest snapshotting (into a C array) at the start is sanest, and lets us avoid problems with the iterator's internal pointer entirely.

[Alexandre Ferrieux]

Donal, you're re-discovering hot water ;-)

The fact that [array unset *] + [trace add ... set ar($newkey) 1] leaves the array with an apparently random subset has been true since Day One of trace !!!

To try to keep the discussion on tracks, two things :

(1) please admit that HEAD currently has a leak that my latest patch fixes, at iso-semantics with Day One.

(2) if we are to iron out the semantics at the cost of *** POTENTIAL INCOMPATIBILITY ***, indeed I've discussed it with Miguel when evaluating "thermonuclear solutions". I was suggesting the snapshot idea, and Miguel proposed to just rewind the iterator. (I still prefer the snapshot, because the rewind leads to quadratic behavior and may fire some unset traces more than once). But the snapshot also has a performance impact wrt current code, and would need to kick in only when unset traces are present (can we detect a single-key trace in O(1) ?).

[Donal K. Fellows]

We can't detect a trace in O(1) as that information is not collected (it can't be collected because we can't go cheaply from the state of knowing that there is a trace to knowing that there isn't one) and just because the semantics have been screwed up for a long time doesn't mean that they should not be fixed.

Your patch is *still* wrong though, independent of that. Really. (Look at the line where you're dealing with the reference count of protectedVarPtr...)

[Donal K. Fellows]

Applied the corrected version of your last patch.

[Alexandre Ferrieux]

At last !!!
Thanks for fixing that "--" typo.