Tcl Source Code

View Ticket
Login
Ticket UUID: 2921116
Title: Segmentation fault when removing the transformation
Type: Bug Version: obsolete: 8.6b1.1
Submitter: sbron Created on: 2009-12-25 16:01:33
Subsystem: 26. Channel Transforms Assigned To: andreas_kupries
Priority: 9 Immediate Severity:
Status: Closed Last Modified: 2010-08-10 00:23:20
Resolution: Fixed Closed By: andreas_kupries
    Closed on: 2010-03-17 16:39:28
Description:
If the read method of a stacked channel (temporarily) removes the transformation to send an acknowledgement back to the sender, Tcl crashes with a segfault.

The problem can be reproduced using the attached script.

Probably the chan pop command releases the data structure attached to the transformation handle that is later still needed to process the data returned by the read method.

Either the code invoking the read method should make a copy of the data it will need to finish processing the read action, or removing the transformation inside the read method should fail with an appropriate error message.

Obviously similar problems may happen in other stacked channel methods when they remove the transformation.

Tested on OpenSuSE 11.2, x86_64
User Comments: andreas_kupries added on 2010-08-10 00:23:20:
Reflected transformation was exposed to scripts in Tcl 8.6.
This does not exist in Tcl 8.5.
Do not confuse with with reflected channel (base channels).

dgp added on 2010-08-10 00:04:22:
any need for backport here?

andreas_kupries added on 2010-03-17 23:39:28:

allow_comments - 1

andreas_kupries added on 2010-03-17 23:39:27:
Fix committed, after adding test cases, and fixing up 'read' equivalent to 'gets'.

dgp added on 2010-03-17 06:04:54:
OK, there may be thread issues, but not apparently
specific to the Preserve/Release of handles.

The ReflectedTransform data structure contains
fields (interp, handle, argv) that want to be freed
in the same thread as created them.  If a ReflectedTransform
can take a ride on a Tcl_Channel from one thread to 
another, that's a design flaw, but not a new one created by
this patch.

dgp added on 2010-03-17 05:52:08:
The patch appears to be a straightforward
conversion to the use of Preserve/Release.
I have not checked for completeness, but the
changes present look sound.

I don't think there are any thread issues.
Tcl_Preserve, etc. operate on global
data structures with mutex protection.
Any thread should have the same ability
as any other to make the Preserve/Release
call on a protected handle.

dgp added on 2010-03-17 05:38:52:
New patch re-synced to the HEAD and
with corrections to the typecasting warnings
in the Tcl_EventuallyFree() calls.

dgp added on 2010-03-17 05:38:17:

File Added - 367035: 2921116.patch

andreas_kupries added on 2010-01-20 02:32:21:
Now attached is a patch which seems to fix the problem.

It uses Tcl_Preserve/Release/EventuallyFree to keep the structure around while the channel driver functions are running. This is done only where we have callbacks which could destroy it.

The ReflectClose function is an exception, because when it is run we are in the destruction already. Doing another destruction in its callbacks (drain. flush, finalize) will very likely still crash us. But the preserve/release looks to be very complex, so I punted on it in this patch.

Part of the problem, and also why I am still uneasy about the patch in general is the behaviour in a threaded environment. More specifically, when the channel with the transform is moved into different thread, and then we have preserve/release on the same structure from different threads, and the eventuallyfree may run in the wrong thread ?!

I might be fretting over nothing here, because:

(1) such a self-closing transform as shown in the example will not work at all when the channel it is part of is moved to a different thread, as the handler code will lose access to the channel (handle), and this will stop such an attempt in its tracks before it even reaches the reflection internals at all.

(2) Moving a transform which does not modify/close itself seems to work. At least the iortrans.tf set of tests does pass, no failures, nor crashes.

Reassigning to dgp for review of my code and reasoning.

andreas_kupries added on 2010-01-20 02:20:41:

File Added - 359556: tcl86.2921116.diff-wu.patch

andreas_kupries added on 2010-01-19 06:21:37:
Ok, having read rttest.tcl now I understand what you are doing. Not clear why this is needed, but the mechanics are clear. You have a transform T stacked on a channel C, and T kills itself during processing of 'read' data, and then regenerates itself, i.e. makes a new copy and stacks it.

I attached my Tcl 2009 paper to your other bug about the seg.fault in a reflected channel due to postevent directly called by watch. I recommed to read it and the slides for transforms as well.

Seesm I have to Preserve/Release the rt structure for a larger block ... Oh, and even so the EventuallyFree is missing, bad.

andreas_kupries added on 2010-01-19 05:25:35:
Popping a transformation does indeed kill it; and this indeed implies that all internal state structures for it are cleaned and freed.

sbron added on 2009-12-25 23:01:34:

File Added - 356445: rttest.tcl

Attachments: