Tcl Source Code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Ticket UUID: 0e4d88b6502ac8fe0ccc912c2bd2ef4c16279837
Title: A trace deletes the namespace when a command is replaced, producing "called Tcl_CreateHashEntry on deleted table"
Type: Bug Version: trunk
Submitter: pooryorick Created on: 2017-08-30 09:20:10
Subsystem: 21. [namespace] Assigned To: dgp
Priority: 5 Medium Severity: Critical
Status: Open Last Modified: 2017-12-21 22:11:27
Resolution: Fixed Closed By: nobody
    Closed on: 2017-09-02 21:48:33
Description:

The following script:

proc deleter {ns args} {
    namespace delete $ns
}

namespace eval n1 {
    proc c1 {} {}
}
trace add command n1::c1 delete [list deleter ::n1]
proc ::n1::c1 {} {}

produces the error:

called Tcl_CreateHashEntry on deleted table
Aborted (core dumped)

User Comments: pooryorick added on 2017-12-21 22:11:27:

The fix for [ba1419303b4c] probably partially addressed the issues described here.


dgp added on 2017-12-06 20:01:43:
Updated status:

8.5 remains broken with valgrind complaining.

8.6 has minimal workaround patch silencing valgrind, but
the foundations are still rotten.

8.7 gets 8.6 treatment for now, but should become the
arena to work out the right solution to these matters.

dgp added on 2017-12-06 17:48:40:
dgp	line 3141 wants cmdPtr->nsPtr to not be freed.
   * pooryorick opens core-8-6-branch...
dgp	the Namespace struct does have a refCount field
<pooryorick>	line 3141, which file?
dgp	tclBasic.c
<pooryorick>	In Tcl_DeleteCommandFromToken?
dgp	heh, you've already fixed this on 8.7 ? 
<pooryorick>	Is was thinking maybe I had.
<pooryorick>	But not in that function...
<pooryorick>	Sounds like it's just about fixed here.
dgp	next thing is to look at the similar patterns
<pooryorick>	In core-8-branch there's something similar in TclReadeObjCommandInNs, which I might have added.
<pooryorick>	But it's still unguarded in Tcl_DeleteCommandFromToken.
dgp	line 1810 is similar, also broken, but not tested?
<pooryorick>	1810 is empty in my checkout, but it's in Tcl_HideCommand
<pooryorick>	I don't see anything in there that would decrement the namespace refCount.
dgp	we have delete and rename traces, but not hide traces, I guess
dgp	Namespace refCounts are not used in nearly as many places as I would have guessed.
dgp	Certainly not by everything that stores an nsPtr.
dgp	Only parent namespaces and ResolvedNsName intreps appear to use it.

dgp added on 2017-12-06 16:23:15:
The memory management brokenness of this work needs fixing for 8.6.8 release.

dgp added on 2017-10-13 14:23:33:
Re-opening this ticket.

The new test basic-15.2 produces invalid reads.

Whether the fix introduced this, or the new
test just exposes it, a proper fix should fix
that as well.

==6980== Invalid read of size 4
==6980==    at 0x423DAA: Tcl_DeleteCommandFromToken (tclBasic.c:2898)
==6980==    by 0x422EA7: Tcl_CreateObjCommand (tclBasic.c:2124)
==6980==    by 0x4E52BB: Tcl_ProcObjCmd (tclProc.c:186)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==    by 0x48B855: TclExecuteByteCode (tclExecute.c:2412)
==6980==    by 0x489936: TclCompEvalObj (tclExecute.c:1533)
==6980==    by 0x42778F: TclEvalObjEx (tclBasic.c:5345)
==6980==    by 0x4E6476: Tcl_UplevelObjCmd (tclProc.c:945)
==6980==  Address 0xc69ee10 is 416 bytes inside a block of size 440 free'd
==6980==    at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6980==    by 0x52EE93: TclpFree (tclAlloc.c:723)
==6980==    by 0x42D5E3: Tcl_Free (tclCkalloc.c:1221)
==6980==    by 0x4C938A: NamespaceFree (tclNamesp.c:1237)
==6980==    by 0x4C9088: Tcl_DeleteNamespace (tclNamesp.c:1042)
==6980==    by 0x4CC09D: NamespaceDeleteCmd (tclNamesp.c:3220)
==6980==    by 0x4CB499: Tcl_NamespaceObjCmd (tclNamesp.c:2845)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==  Block was alloc'd at
==6980==    at 0x4C29BE3: malloc (vg_replace_malloc.c:299)
==6980==    by 0x52EE79: TclpAlloc (tclAlloc.c:700)
==6980==    by 0x42D3E2: Tcl_Alloc (tclCkalloc.c:1058)
==6980==    by 0x4C8BBA: Tcl_CreateNamespace (tclNamesp.c:827)
==6980==    by 0x4CC175: NamespaceEvalCmd (tclNamesp.c:3284)
==6980==    by 0x4CB4D7: Tcl_NamespaceObjCmd (tclNamesp.c:2851)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==    by 0x48B855: TclExecuteByteCode (tclExecute.c:2412)

==6980== Invalid read of size 4
==6980==    at 0x423D81: Tcl_DeleteCommandFromToken (tclBasic.c:2898)
==6980==    by 0x422EA7: Tcl_CreateObjCommand (tclBasic.c:2124)
==6980==    by 0x4E52BB: Tcl_ProcObjCmd (tclProc.c:186)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==    by 0x48B855: TclExecuteByteCode (tclExecute.c:2412)
==6980==    by 0x489936: TclCompEvalObj (tclExecute.c:1533)
==6980==    by 0x42778F: TclEvalObjEx (tclBasic.c:5345)
==6980==    by 0x4E6476: Tcl_UplevelObjCmd (tclProc.c:945)
==6980==  Address 0xc69edd0 is 352 bytes inside a block of size 440 free'd
==6980==    at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6980==    by 0x52EE93: TclpFree (tclAlloc.c:723)
==6980==    by 0x42D5E3: Tcl_Free (tclCkalloc.c:1221)
==6980==    by 0x4C938A: NamespaceFree (tclNamesp.c:1237)
==6980==    by 0x4C9088: Tcl_DeleteNamespace (tclNamesp.c:1042)
==6980==    by 0x4CC09D: NamespaceDeleteCmd (tclNamesp.c:3220)
==6980==    by 0x4CB499: Tcl_NamespaceObjCmd (tclNamesp.c:2845)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==  Block was alloc'd at
==6980==    at 0x4C29BE3: malloc (vg_replace_malloc.c:299)
==6980==    by 0x52EE79: TclpAlloc (tclAlloc.c:700)
==6980==    by 0x42D3E2: Tcl_Alloc (tclCkalloc.c:1058)
==6980==    by 0x4C8BBA: Tcl_CreateNamespace (tclNamesp.c:827)
==6980==    by 0x4CC175: NamespaceEvalCmd (tclNamesp.c:3284)
==6980==    by 0x4CB4D7: Tcl_NamespaceObjCmd (tclNamesp.c:2851)
==6980==    by 0x4251F1: TclEvalObjvInternal (tclBasic.c:3776)
==6980==    by 0x48B855: TclExecuteByteCode (tclExecute.c:2412)

dgp added on 2017-09-02 21:48:33:
Fixed.

dgp added on 2017-09-02 19:37:16:
See branch bug-0e4d88b650 for first draft fix of this
problem in Tcl_CreateObjCommand() on core-8-5-branch.

Needs cleanup and tests, then forward merging.

aspect added on 2017-09-01 09:59:01:
err ..

https://core.tcl.tk/tcl/artifact/45e6cf14bb353bfa?ln=2300

the call to Tcl_DeleteCommandFromToken() invalidates nsPtr, of course, not
whatever I linked above.

aspect added on 2017-09-01 09:57:02:
following this with MEM_DEBUG, nsPtr is being invalidated at:

https://core.tcl.tk/tcl/artifact/45e6cf14bb353bfa?ln=2306

looking at Tcl_DeleteNamespace, I think the fix will require use of nsPtr->refCount and understand the NS_DEAD flag.