[Tcl-bugs] [ tcl-Bugs-571385 ] segfaults in execution tracing.

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Bugs item #571385, was opened at 2002-06-19 19:52
You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=110894&aid=571385&group_id=10894

Category: 17. Commands M-Z
Group: 8.4a5
Status: Open
Resolution: None
Priority: 8
Submitted By: Don Porter (dgp)
Assigned to: Vince Darley (vincentdarley)
Summary: segfaults in execution tracing.

Initial Comment:

assigning to co-author of TIP 62, which introduced this
bug.

I see a segfault in line 4036 of tclCmdMZ.c, apparently due
to an attempt to access tracePtr->nextPtr in a situation
where tracePtr is NULL.

But that's just the beginning...

My code that triggers the segfault is not making use
of execution traces, so it has no business being in
TclCheckExecutionTraces() to begin with.

It's getting there from line 3023 of tclBasic.c by mistake.
Line 3022 checks the value of

    (cmdPtr->flags & CMD_HAS_EXEC_TRACES)

but in my situation cmdPtr is no longer valid because
execution of the command had caused redefinition
of the command, something like:

    proc foo {} {
        proc foo {} {puts NEW}
        uplevel 1 foo
    }

still looking at this to see if I can sort it out.  The
logic of
the trace routines is not transparent.  More comments
would be helpful.

----------------------------------------------------------------------

>Comment By: Don Porter (dgp)
Date: 2002-06-20 11:51

Message:
Logged In: YES 
user_id=80530

Oops!  Failed to attach patch.
Trying again...

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2002-06-20 11:38

Message:
Logged In: YES 
user_id=80530

The attached patch stops the segfaulting
that I see.  I do not know, however, if

1) this depends too much on internal
    details of the tclCmdNameType
    that may change.

2) the patch introduces a memory leak.

----------------------------------------------------------------------

Comment By: Vince Darley (vincentdarley)
Date: 2002-06-20 11:22

Message:
Logged In: YES 
user_id=32170

I don't see from the code how 'cmdPtr' could ever be set 
to null; it's a local variable.

Replacing the main block of code around 3022 with this 
ought to fix a segfault, but may not be an ideal solution:

    /*
     * Finally, invoke the command's Tcl_ObjCmdProc.
     */

    cmdPtr->refCount++;
    iPtr->cmdCount++;
    if ( code == TCL_OK && traceCode == TCL_OK) {
	savedVarFramePtr = iPtr->varFramePtr;
	if (flags & TCL_EVAL_GLOBAL) {
	    iPtr->varFramePtr = NULL;
	}
	code = (*cmdPtr->objProc)(cmdPtr-
>objClientData, interp, objc, objv);
	iPtr->varFramePtr = savedVarFramePtr;
    }
    if (Tcl_AsyncReady()) {
	code = Tcl_AsyncInvoke(interp, code);
    }

    /*
     * Call 'leave' command traces
     */
    if (!cmdPtr->flags & CMD_IS_DELETED) {
	if ((cmdPtr->flags & CMD_HAS_EXEC_TRACES) 
&& (traceCode == TCL_OK)) {
	    traceCode = TclCheckExecutionTraces
(interp, command, length,
			cmdPtr, code, 
TCL_TRACE_LEAVE_EXEC, objc, objv);
	}
	if (iPtr->tracePtr != NULL && traceCode == 
TCL_OK) {
	    traceCode = TclCheckInterpTraces(interp, 
command, length,
			cmdPtr, code, 
TCL_TRACE_LEAVE_EXEC, objc, objv);
	}
    }
    TclCleanupCommand(cmdPtr);

----------------------------------------------------------------------

Comment By: Hemang Lavana (hemanglavana)
Date: 2002-06-20 11:14

Message:
Logged In: YES 
user_id=81875

1. yes please add me to the list of maintainers

2. I have sent the patch to Don (direct e-mail) to see if it resolves 
    the problem

3. cmdPtr should either have NULL value or a valid value. It
    should never point to an invalid address. If the memory
    for cmdPtr has been re-allocated, then cmdPtr should be
    set to null (l will look into this).

4. sf does not allow me to attach files today.

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2002-06-20 11:10

Message:
Logged In: YES 
user_id=80530

no joy.  we need something witht the refCount to stop
the re-allocation I think.

Perhaps we can keep around a Tcl_Obj that holds
a reference to the Command structure?

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2002-06-20 10:53

Message:
Logged In: YES 
user_id=80530

A copy of the report came in e-mail with a usable form
of the patch.  Trying now...

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2002-06-20 10:42

Message:
Logged In: YES 
user_id=80530

several responses:

1) The Command structure has its own refCount field, so
Tcl_Preserve
    should not be used on pointers to it.

2) We should put Hemang on the list of maintainers who can
be assigned
     Tcl Bugs.

3) I agree that (cmdPtr->flags & CMD_HAS_EXEC_TRACES) should
    not be true if the code was correct.  By the time that
expression is
    computed, though, it seems that the Command structure
pointed to
    has been freed and the memory re-allocated to another
purpose.
    I see a (faulty) value of cmdPtr->flags == 1852793951 @
line 3022
    of tclBasic.c just before a segfault in
TclCheckExecutionTraces().

4) if cmdPtr is NULL, I expect cmdPtr->flags access to segfault.

5) Sadly I have not been able to create a short demo script,
though
    I can reliably reproduce the problem with OOMMF.  The proc
    example was just meant to illustrate the kind of thing
that's going
    on.  It is not a demo script.

6) As far as correct behavior goes, whatever was specified
in the TIP.

7) SF won't let you attach the patch, will it? Grrrr.....
can you mail it?
    dg...@us...  I'm happy to try it, but the form
mangled by
    comment processing is nearly unusable.

8) This is a total showstopper for OOMMF.  It will be very
bad if this
    bug gets into the Tcl 8.4b1 release.

----------------------------------------------------------------------

Comment By: Hemang Lavana (hemanglavana)
Date: 2002-06-20 09:02

Message:
Logged In: YES 
user_id=81875

Since there are no execution traces involved, 

(cmdPtr->flags & CMD_HAS_EXEC_TRACES) 

should never be true unless cmdPtr is pointing
to arbitrary memory location. cmdPtr may be
NULL or even if cmdPtr is invalidated, the above
condition should not be true. So, we need to
first figure out, why this bit was set.

I could not replicate this problem on solaris2.6:
godel:76> /tmp/hlavana/tcltk/bin/tclsh8.4
% info patch
8.4a5
% proc foo {} {
proc foo {} {puts NEW}
uplevel foo
}
% foo
NEW
% exit
godel:77>

Don, can you provide the script that gives seg. fault.

Next question is: what should be its behavior
when execution traces are set and the command
has been modified in some way. The execution
traces should be disabled.

Although I am not very familiar with 'cmdPtr->cmdEpoch',
a cursory look at the code tells me that it is sufficient to
check whether 'cmdPtr->cmdEpoch' has been modified
or not (it covers CMD_IS_DELTED case as well). I am
providing the following patch to prevent 'leave' traces
from being invoked when the command itself has been
modified.

godel:80> cvs diff tclBasic.c 
Index: tclBasic.c
===================================================================
RCS file: /cvsroot/tcl/tcl/generic/tclBasic.c,v
retrieving revision 1.61
diff -r1.61 tclBasic.c
2927a2928
>     int cmdEpoch;
2974a2976,2985
>          * According to tclInt.h, cmdPtr->cmdEpoch is incremented 
>          * to invalidate any references that point to this command 
>          * when the it is renamed, deleted, hidden, or exposed. 
>          * Save the current value of cmdPtr->cmdEpoch, so that we
>          * can use it later on to find out if the command has been
>          * modified in any way or not.
>          */
>         cmdEpoch = cmdPtr->cmdEpoch;
> 
>         /*
2978d2988
<             int cmdEpoch = cmdPtr->cmdEpoch;
3021c3031,3032
<      * Call 'leave' command traces
---
>      * If the command has been modified in any while executing the code, 
>      * then do not call any traces.
3023,3025c3034,3035
<     if ((cmdPtr->flags & CMD_HAS_EXEC_TRACES) && (traceCode == TCL_OK)) {
<       traceCode = TclCheckExecutionTraces(interp, command, length,
<                      cmdPtr, code, TCL_TRACE_LEAVE_EXEC, objc, objv);
---
>     if (cmdEpoch != cmdPtr->cmdEpoch) {
>         checkTraces = 0;
3027,3028c3037,3043
<     if (iPtr->tracePtr != NULL && traceCode == TCL_OK) {
<       traceCode = TclCheckInterpTraces(interp, command, length,
---
> 
>     /*
>      * Call 'leave' command traces, if needed.
>      */
>     if ((checkTraces) && (command != NULL)) {
>         if ((cmdPtr->flags & CMD_HAS_EXEC_TRACES) && (traceCode == TCL_OK)) {
>           traceCode = TclCheckExecutionTraces(interp, command, length,
3029a3045,3049
>         }
>         if (iPtr->tracePtr != NULL && traceCode == TCL_OK) {
>           traceCode = TclCheckInterpTraces(interp, command, length,
>                      cmdPtr, code, TCL_TRACE_LEAVE_EXEC, objc, objv);
>         }
godel:81> 

----------------------------------------------------------------------

Comment By: Vince Darley (vincentdarley)
Date: 2002-06-20 06:58

Message:
Logged In: YES 
user_id=32170

Looks as if something like the logic which is applied 20 
lines up needs to be applied, but we still need 
the 'cmdPtr' to be around.  I think we need to 
Tcl_Preserve the cmdPtr, if traces are going to be called, 
and then Tcl_Release afterwards.

Not sure if the correct afterwards test is to check 
if 'cmdPtr->cmdEpoch' has changed or if cmdPtr has 
been deleted.  I think the latter is the right one (cmdPtr-
>flags & CMD_IS_DELETED).  Partly this decision 
depends on the desired semantics of execution traces 
when the command is modified.  According to tclInt.h, 
cmdPtr->cmdEpoch is incremented when the command 
is renamed, deleted, hidden, or exposed.  What do we 
want execution traces to do in these cases?

Vince.

I think Hemang ought to take a look at this, since he's 
been examining the trace code recently, while I haven't 
looked at it in a very long time (1 year +).

----------------------------------------------------------------------

You can respond by visiting: 
http://sourceforge.net/tracker/?func=detail&atid=110894&aid=571385&group_id=10894

[Tcl-bugs] [ tcl-Bugs-571385 ] segfaults in execution tracing.

The Tool Command Language implementation

[Tcl-bugs] [ tcl-Bugs-571385 ] segfaults in execution tracing.