From: <no...@so...> - 2002-06-20 16:44:14
|
Bugs item #571385, was opened at 2002-06-19 20:52 You can respond by visiting: http://sourceforge.net/tracker/?func=detail&atid=110894&aid=571385&group_id=10894 >Category: 43. Parsing and Eval Group: 8.4a5 Status: Open Resolution: None >Priority: 3 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: miguel sofer (msofer) Date: 2002-06-20 13:44 Message: 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? ---------------------------------------------------------------------- Comment By: Hemang Lavana (hemanglavana) Date: 2002-06-20 13:33 Message: 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 ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2002-06-20 13:23 Message: 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) ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-06-20 13:11 Message: 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. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-06-20 13:01 Message: Logged In: YES user_id=80530 sorry, that patch got mangled. Here's a replacement. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-06-20 12:53 Message: Logged In: YES user_id=80530 Here's Darley's patch as diff against HEAD. It also stops the segfaults. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-06-20 12:51 Message: Logged In: YES user_id=80530 Oops! Failed to attach patch. Trying again... ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2002-06-20 12: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 12: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 12: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 12: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 11: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 11: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 10: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 07: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 |