Tcl Source Code

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.

Logged In: YES 
user_id=32170

Fixed and made trace routines somewhat more transparent.

Logged In: YES 
user_id=32170

Definitely keep this open, and I would _really_ like a 
test which illustrates the previous bug, since, although 
the 'darley patch' works, that 20-30 lines of code really 
should be made clearer and more streamlined, and I'd 
like to do that (when I have time) without a chance of re-
introducing the bug.

Logged In: YES 
user_id=80530


I agree we should keep this open as a reminder to
add tests for the bug.

Also, note that the coding of
TclCheckExecutionTraces() appears to allow
for a potential segfault in line 4036, as noted
in the original report.  Someone ought to look
that over.

File Deleted - 25497: 



File Added - 25501: darley.patch

Logged In: YES 
user_id=148712

Indeed; patch corrected here and committed to HEAD. I'm
leaving this open at lowered priority: did anybody find a
test for this bug?

Logged In: YES 
user_id=81875

Please add paratheses around the check for CMD_IS_DELETED flag:

if (!(cmdPtr->flags & CMD_IS_DELETED)) {

After this change it passes all the trace tests

Logged In: YES 
user_id=148712

Patch looks fine to me, *but* it causes failures in
trace.test 21.(3-8), 23.(1-3), 24.(2-5), 25.2 , 25.(5-7)

Logged In: YES 
user_id=80530

Hemang and I agree that darley's patch seems to
be the best immediate fix.  Assigning to
maintainer for evaluation and commit.

File Deleted - 25495: 



File Added - 25497: darley.patch

Logged In: YES 
user_id=80530

sorry, that patch got mangled.
Here's a replacement.

File Added - 25495: darley.patch

Logged In: YES 
user_id=80530


Here's Darley's patch as diff against HEAD.

It also stops the segfaults.

File Added - 25494: extr.patch

Logged In: YES 
user_id=80530

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

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.

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);

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.

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?

Logged In: YES 
user_id=80530


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

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?
    [email protected]  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.

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>

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 +).