Tcl Source Code

Please see the attached testcase (tcltest.tar.gz)

Problem Analysis:
----------------
The problem happens when a C function calls a TCL proc that calls a C function that returns TCL_RETURN, this causes returnLevel to be decremented all the way to 0 causing TclUpdateReturnInfo() to panic.

Testcase:
--------
 + (tcltest.c): main() calls CProcCmd() function which is a C function with a TCL wrapper.
 + CProcCmd() calls tclproc (tcltest.tcl) which calls CProcCmd() again.
 + CProcCmd() returns TCL_RETURN which makes TCL panic.

Workarounds:
-----------
There are two workarounds that I can use to make the testcase work (uncomment either line 25 or 28 in tcltest.c):

+ Workaround 1 (line 25): 
Explicitly increment returnLevel before returning TCL_RETURN.

+ Workaround 2 (line 28): 
Call a dummy tcl proc before returning TCL_RETURN. This works because it forces the returllevel to be incremented once and thus stops it from ever reaching 0.

committed for 8.5.6 and 8.6b1

even simpler patch attached.
File Added: 2152286.patch

File Added - 297976: 2152286.patch

diff -u -r1.139.2.3 tclProc.c
--- generic/tclProc.c   11 Aug 2008 19:01:59 -0000      1.139.2.3
+++ generic/tclProc.c   19 Oct 2008 19:30:06 -0000
@@ -2245,6 +2245,10 @@
        if (code == TCL_ERROR) {
            iPtr->flags |= ERR_LEGACY_COPY;
        }
+       if (code != TCL_RETURN) {
+           iPtr->returnLevel = 1;
+           iPtr->returnCode = TCL_OK;
+       }
     }
     return code;
 }

Ok, none of my last comment
is wrong, exactly, but it
did miss the point.  Whether
or not C code calling Tcl
should do what the demo does, it's
now apparent that a small,
simple, safe change to Tcl
itself will permit it to
tolerate such code, and that's
good for all.

Testing that change now...

The error here is in the evaluation
of [cproc 1].  With a single argument
the branch is taken that evaluates
[tclproc].  If you examine the return
code from that Tcl_Eval(), you will
find that it is TCL_OK.  The command
[tclproc] has run successfully.

You then turn around and originate a
TCL_RETURN return code, without
dealing with the fact that an
intervening Tcl_Eval() has taken
the interp out of the reset state
it had when it entered CProcCmd.

There are a number of ways to
fix the code, depending on just
what it is this code is intended
to do.  That is not clear at all
from the example, which feels 
contrived to demo the conditions
for panic, more than directed
toward any rational purpose.  That's
fine; that's often true of bug
demos, but leaves me without any
context to choose what alternative
might make sense.

If you have a Tcl_ObjCmdProc that
will return TCL_RETURN, you should
either do that with the interp
still in its original reset state,
or do that after something you called
also returned TCL_RETURN (the "pass
along the exception" case), or take
care to call Tcl_SetReturnOptions()
to set the relevant options as needed
for your command (the "I'm originating
TCL_RETURN for my own purposes" case).

It seems to me the panic is serving
exactly the purpose intended, to call
attention to C code that's not robust
or safe so it can get corrected.

Note that your workarounds are just
tricky indirect ways to take my advice.
Explicit returnLevel increment is just
a sneaky "probe the internals" way to
accomplish what a proper Tcl_SetReturnOptions()
call would do.  The dummy proc call is just
a very indirect way to call the
Tcl_ResetResult() you need to get the
interp back to an initial state where
simple return of TCL_RETURN will cause
no trouble.

Hope that helps.  I'll keep this report
pending in case you want to comment
further or seek advice.

A quick attempt to patch up that
flaw does not resolve the issue, 
so I'll examine further.  In the meantime,
do not confuse the coding of 
procs, which push CallFrames, and
non-proc commands, which do not.

The flaw here is in the CProcCmd()
routine.  Several places in that routine
Tcl_Eval is called on some script, but
the return code is not checked.  If you
were to check it, you would find the
value to be TCL_RETURN, which is typically
an indication that you should not just
continue normally.  Your code continues
normally.

File Deleted - 296502: 

Please use the .zip file

That file seems to be corrupt:

mig@oli:/tmp$ tar -ztf /tmp/tcltest.tar.gz 
tar: This does not look like a tar archive
tar: Skipping to next header
tar: Error exit delayed from previous errors

File Added - 296503: tcltest.zip

File Added: tcltest.zip

File Deleted - 296501: 



File Added - 296502: tcltest.tar.gz

File Added: tcltest.tar.gz

File Added - 296501: tcltest.tar.gz