Summary

Transfering a channel to another thread by "thread::transfer" with a "fileevent writable" on the channel crashes tclsh after around 20 tries on Linux and Windows. The crash goes away, if the file event is cleared before the transfer.

Crash

Test script:

package require Thread

set handleproc {
    proc handle {chan} {
        global name other
        if {$name eq "master"} {
            puts $name
        } else {
            thread::send $other "puts slave"
        }
        thread::transfer $other $chan
        thread::send $other [list fileevent $chan writable [list handle $chan]]
    }
}

eval $handleproc
set name master
set other [thread::create]

thread::send $other [list set name slave]
thread::send $other [list set other [thread::id]]
thread::send $other $handleproc

set nulldev [dict get {unix /dev/null windows NUL}  $tcl_platform(platform)]
set chan [open $nulldev RDWR]
handle $chan

vwait forever 

Demonstration that it works without fileevent active

The following script switches the event of before the transfer. It does not crash:

package require Thread

set handleproc {
    proc handle {chan} {
        global name other
        if {$name eq "master"} {
            puts $name
        } else {
            thread::send $other "puts slave"
        }
        thread::transfer $other $chan
        thread::send $other [subst {
            fileevent $chan writable {
                fileevent $chan writable {}
                after 0 handle $chan
            }
        }]
    }
}

eval $handleproc
set name master
set other [thread::create]

thread::send $other [list set name slave]
thread::send $other [list set other [thread::id]]
thread::send $other $handleproc

set nulldev [dict get {unix /dev/null windows NUL}  $tcl_platform(platform)]
set chan [open $nulldev RDWR]
handle $chan

vwait forever 

References

• Discussion thread on the core list titled: "TCL (Windows) socket driver and sending fd to another Thread" started 2018-06-07
• Bug report and test script by Reinhard Max
• cause analysis by Christian Werner

It looks like this ticket in thread-repo [d481f15516a465b1] may be affected by this issue too.

I tried to test the fix without atomic operations (rewritten atomic-defines similar the last section from current branch if no TCL_THREADS declared, so as (++(p)) and (--(p))).

It's not stable as non-atomic (sporadically produces panic "Channel released more than preserved").
So no way without - either atomic or mutex (which IMHO a bit superfluous for incr/decr).

Please find enclosed better (faster) test-case without hand-brake in the console (so it reproduces very fast a segfault or panic in original tcl without the fix, resp. without atomic operations also):

set handleproc {
    proc handle {chan} {
        global name other cntr
        if 0 {;#
            if {$name eq "master"} {
                puts -nonewline M
                if {!([incr cntr] % 100)} {flush stdout}
            } else {
                thread::send $other {puts -nonewline s}
            }
        } else {
            if {!([incr cntr] % 50)} {
                if {$name eq "master"} {
                    puts -nonewline [expr {$cntr % 2500 ? "" : " -- $cntr\n"}]M
                } else {
                    thread::send $other {puts -nonewline s}
                }
            }
        };#
        if 0 {
            thread::transfer $other $chan
        } else {
            thread::detach $chan
            #thread::send $other [list thread::attach $chan]
            thread::send -async $other [list thread::attach $chan]
        }
        thread::send $other [list fileevent $chan writable [list handle $chan]]
    }
}

eval $handleproc
set name master
set other [thread::create]

thread::send $other [list set name slave]
thread::send $other [list set other [thread::id]]
thread::send $other $handleproc

set nulldev [dict get {unix /dev/null windows NUL}  $tcl_platform(platform)]
set chan [open $nulldev RDWR]
handle $chan

vwait forever

Hi Harald, Thx for test! Still someone with *nix?:)

And the question remains - what we do with atomic builtins?
Does anyone have some special system/platform (older compiler) to test?

Sergey, thank you for your continuous passionous work. The fix works totally for me.

Test details

Here is my windows test on Win10 2017-10 64 bit GER compiled with 32 bit:

• Download tarbal from [aacd6f9233]
• Unzip to C:\test\tcl-aacd6f9233
• Copy tcl8.6.8 thread package to pkgs: c:\test\tcl8.6.8\pkgs\thread2.8.2 -> c:\test\tcl-aacd6f9233\pkgs
• Start cmd prompt and run VCVARS32.bat of MS-VC6 and "setenv.cmd /RETAIL" of PSDK2003SP1.
• cd C:\test\tcl-aacd6f9233\win
• nmake -f Makefile.vc
• nmake -f Makefile.vc install INSTALLDIR=c:\test\tcl8.6.8_sebres
• cd c:\test\tcl8.6.8_sebres\bin
• Put the test script of the initial report to c:\test\testsocketthread.tcl
• tclsh86t.exe c:\test\testsocketthread.tcl

The crash does not occure after 1 Minuite run time. With original 8.6.8, it occures after 20 iterations.

TCL Test run:

• nmake -f Makefile.vc test TESTFLAGS="-verbose bte" > testlog_tcl8.6.8_815e246806_2018-06-12.txt 2>&1

compared to tcl 8.6.8 release (http://wiki.tcl.tk/37529), there are no test differences.

Sergey, great work, thank you, Harald

Branch fix-815e246806-8-6 is ready now for testing/review.

This fix could theoretacally work without the new handling round abount channel ref-count (also non atomic), but... I'm not sure it works then in all possible constellations. In addition I would like to save this code to be pretty sure if someone calls this public API across two threads a bit other way (as in thread-module).

And just to have atomic operations in tcl for future requirements (I've used it already for windows since I fixed pipes-error [8bd13f07bd], see [ad65e576ee]).

Just this handling (atomic primitives in tclInt.h) is not complete possibly and may be missing for some older gcc/clang versions resp. some special platforms, that are don't have support for builtin atomic (or just miss some header include).
If so, we should extend this on demand (similar way other projects doing this, e. g. we've this in nginx, see ngx_atomic_fetch_add in ngx_atomic.h for example.
Or just to extend it with non-atomic variants of TclAtomicFetch... if totally unsupported.

ATM tested MS-VC (>= 6.0), mingw (gcc >= 4.2), linux (gcc >= 6.2, clang >= 3.8).

The case if channel switches owner via "thread::transfer" fixed now in [2e7300065a].

Currently still as interim state, because thread::detach/attach still failed sporadically with "SIGBUS":

Thread 2 "tclsh" received signal SIGBUS, Bus error.
[Switching to Thread 0x7ffff62bc700 (LWP 20721)]
0x00007ffff7b089c3 in TclChannelEventScriptInvoker () from ./libtcl8.6.so

#0  0x00007ffff7b089c3 in TclChannelEventScriptInvoker () from ./libtcl8.6.so
#1  0x00007ffff7b0da0f in Tcl_NotifyChannel () from ./libtcl8.6.so
#2  0x00007ffff7b69a77 in FileHandlerEventProc () from ./libtcl8.6.so
#3  0x00007ffff7b280cf in Tcl_ServiceEvent () from ./libtcl8.6.so
#4  0x00007ffff7b283f4 in Tcl_DoOneEvent () from ./libtcl8.6.so
#5  0x00007ffff62c05df in ThreadWaitObjCmd () from ./libthread2.9a1.so
#6  0x00007ffff7a61827 in TclNRRunCallbacks () from ./libtcl8.6.so
#7  0x00007ffff7a63932 in TclEvalEx () from ./libtcl8.6.so
#8  0x00007ffff7a63ee3 in Tcl_EvalEx () from ./libtcl8.6.so
#9  0x00007ffff62c2502 in NewThread () from ./libthread2.9a1.so
#10 0x00007ffff70236ca in start_thread (arg=0x7ffff62bc700) at pthread_create.c:333
#11 0x00007ffff77600af in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105

Stack level 0, frame at 0x7ffff62bbc60:
 rip = 0x7ffff7b089c3 in TclChannelEventScriptInvoker; saved rip = 0x7ffff7b0da0f
 called by frame at 0x7ffff62bbcb0
 Arglist at 0x7ffff62bbc18, args:
 Locals at 0x7ffff62bbc18, Previous frame's sp is 0x7ffff62bbc60
 Saved registers:
  rbx at 0x7ffff62bbc38, rbp at 0x7ffff62bbc40, r12 at 0x7ffff62bbc48, r13 at 0x7ffff62bbc50, rip at 0x7ffff62bbc58

But strange is trying to simulate thread::transfer, using "send -async" by attach, ends into SIGSEGV with almost the same call-stack inside TclChannelEventScriptInvoker.

I doubt it may be something wrong with "detach", because it does definitelly the owner thread.

WiP

Too complex to port my tcl-thread-module back (too many dependencies to resolve)...

But just tried to protect the refCount of channel with atomic operations and it solves the crashes for windows (8.5/8.6), but on unix it refuses for some reasons for 8.6 (but work in 8.5).

So tried to add assert within TclChannelEventScriptInvoker to check Tcl_GetCurrentThread() == Tcl_GetChannelThread(chanPtr), and it fails, as expected, with:

    [8F63700]/[AC79700] invoke [0x55cd0c822df0]: handle file5

    if (Tcl_GetCurrentThread() != Tcl_GetChannelThread(chanPtr)) {
	fprintf(stderr," [%X]/[%X] invoke [%p]: %s\n", 
		(int)Tcl_GetCurrentThread(),
		(int)Tcl_GetChannelThread(chanPtr), chanPtr,
		Tcl_GetString(esPtr->scriptPtr));
    }

Still worse is, that if I replace the transfer with attach/detach pair like below, it changes nothing at all:

    if 0 {
	thread::transfer $other $chan
    } else {
	thread::detach $chan
	thread::send $other [list thread::attach $chan]
    }

This implies the "detach" does not work properly (I mean after detach TclChannelEventScriptInvoker will be called again inside the OLD thread).

Strange is also, that it's absolutely not reproducible with my own thread-module (but still the same tcl-version)...

So now it going to the thread-module (tested with 2.7 up to 2.8.1 resp. current trunk).

I had a fixed (with rewritten attach/detach/transfer) version installed where it does not occur.
After change to tcl-core version it's pretty reproducible.

Trying to port bug-fix... WiP.

Strange... I cannot reproduce it anymore.
Just to be sure, which version of the tclthread-module do you use?
Or better revision if self-compiled...

Unfortunately I see no change in behavior between 8.6.7 and fix-815e246806-8-6.
I still see get the crashes with the "writable" script from Harald's initial description and also with the "readable + delayed" one from my latest post (after replacing [after idle] by [eval]).

BTW, most invocations (pre and post your patch) result in a plain segfault without further output, but sometimes I get an error message + abort with differing error messages, e.g.:

----
alloc: invalid block: 0x5645d07a60b0: c0 d0
alloc: invalid block: 0x5645d07a60b0: c0 d0
----
TclStackFree: incorrect freePtr (0x55807a654ac0 != 0x55807a654b00). Call out of sequence?
----
TclNRExecuteByteCode: abnormal return at pc 6: stack top -18 < entry stack top 0
TclNRExecuteByteCode execution failure: end stack top < start stack top
----
Reuse of ChannelBuffer! 0x55ffee568d80
Reuse of ChannelBuffer! 0x55ffee568d80
----
Tcl_AppendLimitedToObj called with shared object
----

This looks to me like there is still some severe memory corruption going on.

Two new branches "fix-815e246806-8-5" and "fix-815e246806-8-6" provide a fix for the issue, see [f44a1ddd60] for example.
Ceched for bot win and nix - neither the panic nor the crash occur now.
All test-cases also passed on all my platforms.

Further testers and reviewers are welcome.

Yep, the reason for it is the pair TclChannelPreserve/TclChannelRelease in the TclChannelEventScriptInvoker.

    TclChannelPreserve((Tcl_Channel)chanPtr);
       Tcl_EvalObjEx(interp, esPtr->scriptPtr, ...); etc...
    TclChannelRelease((Tcl_Channel)chanPtr);

Hmm strange, I can't reproduce it on my own tcl-versions at all (but pretty good reproducible with original core-tcl >= 8.5).
I'll try to find what is different there as regards the fileevents.
Could be possibly fixed via sebres-8-x-event-perf-branch's, that are merged on my own tcl's.
I'll supply a fix if I find the reason.

Here is another variant that shows two things:

1. It also happens with readable events
2. It is the setup of the new fileevent before the old handler has returned that triggers it, not the actual triggering of the event in the other thread. In other words, even if the triggering of the new event handler is delayed the crash still happens if the fileevent has been set up before the old handler had returned.

I post the variant that works. The crash happens when [after idle] in the handle proc is removed or replaced by [eval].

set handleproc {
    proc handle {chan} {
	global name other
	puts $name
	read $chan
	thread::transfer $other $chan
	after idle [subst {
	    thread::send $other {
		fileevent $chan readable {handle $chan}
	    }
	}]
	thread::send $other [list after 500 [list puts $chan x]]
    }
}

eval $handleproc
set name master
set other [thread::create]
set chan [open |cat RDWR]
fconfigure $chan -buffering none -blocking 0

thread::send $other [list set name slave]
thread::send $other [list set other [thread::id]]
thread::send $other $handleproc

handle $chan

vwait forever

The error escape observed on windows is in line 1974 in generic/tclIO.c :

void
TclChannelRelease(
    Tcl_Channel chan)
{
    Channel *chanPtr = (Channel *) chan;

    if (chanPtr->refCount == 0) {
--->    Tcl_Panic("Channel released more than preserved");
    }
    if (--chanPtr->refCount) {
        return;
    }
    if (chanPtr->typePtr == NULL) {
        ckfree(chanPtr);
    }
}

The fileevent appears to get cleared automatically when transferring the channel, so there is no need to do that manually before the transfer.

The critical point for the crash seems to be that a new fileevent is being set up in the receiving thread while the event handler in the sending thread is still running. So, if I just warp the last line of the handler proc in my original script in

 after idle [list ...]

to defer setting up the new event trigger until the old handler has returned, the crash is gone.