Tcl Source Code

When an unset trace upon namespace deletion (TclDeleteNamespaceVars) brings back a namespace variable (ie. defines it by assigning a value), the corresponding Var(InHash) is not freed:

==32261== 64 bytes in 1 blocks are definitely lost in loss record 3 of 3
==32261==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==32261==    by 0x4E75392: TclpAlloc (tclAlloc.c:699)
==32261==    by 0x4E8CE23: Tcl_Alloc (tclCkalloc.c:1059)
==32261==    by 0x4FE532C: AllocVarEntry (tclVar.c:6013)
==32261==    by 0x4F704D7: CreateHashEntry (tclHash.c:359)
==32261==    by 0x4FDC8AD: VarHashCreateVar (tclVar.c:63)
==32261==    by 0x4FDD7A4: TclLookupSimpleVar (tclVar.c:853)
==32261==    by 0x4FDD130: TclObjLookupVarEx (tclVar.c:615)
==32261==    by 0x4FE2E09: Tcl_VariableObjCmd (tclVar.c:4617)
==32261==    by 0x4E80709: Dispatch (tclBasic.c:4358)
==32261==    by 0x4E8078F: TclNRRunCallbacks (tclBasic.c:4391)
==32261==    by 0x4E80065: Tcl_EvalObjv (tclBasic.c:4121)

To reproduce:

1) apply patch
2) run:

valgrind --leak-check=full --track-origins=yes ./tcltest /usr/local/src/tcl/tests/all.tcl -singleproc 1 -file var.test

(see the test case var-8.3 added to var.test by the patch attached)

The proposed fix is to call TclSetVarUndefined(varPtr) after having triggered any traces, as hinted at by the existing, but unimplemented comment:

"[...] force it *undefined* in case an unset trace brought it back from the dead."

Without that, the indirectly called FreeVarEntry() will not free the revived Var in question.
As far as I can judge, this affects *all* Tcl versions 8.5.*, 8.6.*, and 8.7.*, as the code in question appears unaltered to me between major versions.

The unset trace (in this test) also gives rise to a LiteralEntry leak (due BC'ing the unset proc):

==32261==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==32261==    by 0x4E75392: TclpAlloc (tclAlloc.c:699)
==32261==    by 0x4E8CE23: Tcl_Alloc (tclCkalloc.c:1059)
==32261==    by 0x4F9DD86: TclCreateLiteral (tclLiteral.c:236)
==32261==    by 0x4F9E0F4: TclRegisterLiteral (tclLiteral.c:434)
==32261==    by 0x4EE96FF: TclCompileSetCmd (tclCompCmdsSZ.c:218)
==32261==    by 0x4F4B4E8: TclAttemptCompileProc (tclEnsemble.c:3212)
==32261==    by 0x4F2A21A: CompileCmdCompileProc (tclCompile.c:1950)
==32261==    by 0x4F2A820: CompileCommandTokens (tclCompile.c:2065)
==32261==    by 0x4F2ACF4: TclCompileScript (tclCompile.c:2196)
==32261==    by 0x4F2BF9A: TclCompileTokens (tclCompile.c:2445)
==32261==    by 0x4EB093D: TclCompileConcatCmd (tclCompCmds.c:816)

Don't know yet how to fix that one (as the literal machinery such as TclReleaseLiteral is not available in this context, and should not be, most probably).

Thx Don for taking care of this. I can confirm that the leaks are effectively plugged this way (tested with valgrind also).

One follow-up question: 

TclDeleteNamespaceVars is for namespaced vars specifically. Provided that an extension uses TclInitVarHashTable to provide for custom var tables (e.g., for objects without namespaces as in NSF/NX), TclDeleteVars (on cleanup) does not come with an equivalent, now fixed logic to deal with reviving unset traces. 

The doc on TclDeleteVars says "For this function to work correctly, it must not be possible for any of the variables in the table to be accessed from Tcl commands (e.g. from trace functions)."

Mmmmh. So for extensions, one is left with a leaking delete procedure (TclDeleteVars)?

Fixed on all branches

See branch bug-4dbdd9af14 for proposed fix.
Tests to be added before merge.

Bisecting points the actual blame here:

http://core.tcl.tk/tcl/info/e8cc729

Here's a demo script:

while 1 {
  namespace eval n {
    variable v 123
    trace variable v u ::t
  }
  proc t {a i o} {
    set $a 321
  }
  namespace delete n
}

Monitor to check whether memory grows.
(Or adapt to use [memory])

This shows that the leak is not the fault
of VarReform.  This script leaks even before
that.

Hi Don,

I don't see how the Var leak could be caused by the 0083565512bf7409 checkin, which essentially adds Tcl_GetVariableFullName to TclDeleteNamespaceVars.

Rather, the previously present TclSetVarUndefined (ie. prior to checkin 2fbb34460171f3c1) was accidentally removed, so the revived Var fails to be freed in FreeVarEntry because of TclIsVarUndefined == FALSE ...

As for the Literal leak: Shouldn't it be prohibited to register/create a literal in a deletion/shutdown situation?

This is lingering fallout from the 8.5 era VarReform, yes.

However, the VarReform checkin identified by aspect
did not create this leak.  Rather it appeared in checkin

http://core.tcl.tk/tcl/info/0083565512bf7409

which was a fix for

http://core.tcl.tk/tcl/tktview?name=1911919

(and which also introduced test var-8.2).

I have suspicion that this fix may have been
off the mark.

This is difficult stuff, and fortunately rarely encountered.

Both leaks have the same root cause: the failure
to unset and destroy the variable that was
revived by the trace.

This leaks both the structure used to implement
the variable, and the value stored in that variable
(which happened to be created as a literal in this
example), since failure to unset left its refcount
elevated.

Confirmed both leaks.

Please explain "proliferation of literals" .

Thx for this pointer, I missed that.

Given that the var reform 2007 predates the proliferation of literals as in 8.6, it is not a source of inspiration for the second leak, I am afraid.

The patch looks like it restores what went missing in [http://core.tcl.tk/tcl/info/2fbb34460171f3c1].