Tcl Source Code

View Ticket
Login
Ticket UUID: 3422267ed6b79922ca6a69883d4e39901516443e
Title: segmentation fault from deleting the the target of an imported alias during a trace on the target of the alias
Type: Bug Version: 8.7
Submitter: pooryorick Created on: 2020-08-12 12:54:58
Subsystem: 21. [namespace] Assigned To: pooryorick
Priority: 5 Medium Severity: Minor
Status: Open Last Modified: 2020-09-06 14:50:07
Resolution: None Closed By: nobody
    Closed on: 2020-08-29 21:50:41
Description:

The following script produces a segmentation fault in an interpreter built with -DPURIFY and --enable-symbols-debug:

variable res {}

proc p1 args {
    return success
}
namespace eval ns1 {
    namespace export *
}
interp alias {} [namespace current]::ns1::p2 {} [namespace current]::p1
namespace eval ns2 {
    namespace import [namespace parent]::ns1::p2
}
proc ondelete {oldname newname op} {
    variable res
    namespace delete ns1
    catch {
        ns1::p2
    }  res
}

trace add command ns2::p2 delete [namespace which ondelete]
rename ns2::p2 {}
rename p1 {}

The expected result is an error message about ns1::p2

invalid command name "ns1::p2"

The issue is that in tclNamesp.c:DeleteImportedCmd, realCmdPtr points to memory that has already been deallocated. For imported commands reference counting and cleanup is currently incomplete.

User Comments: jan.nijtmans added on 2020-09-06 14:50:07:

Well, the Travis build of this branch failed again, turns out it's still not correct. So, I merged in the latest 8.7, so you can try again.

Current symptom:

interp.test
Test file error: child killed: segmentation violation

See: https://travis-ci.org/github/tcltk/tcl/builds/724602003


jan.nijtmans added on 2020-09-05 21:41:03:

Sorry, I was a little to quick reverthing this fix. Looks OK!

The actual cause of the memory leak was [c1a376375e], an earlier merge to core-8-branch which was too early.

Please, wait until core-8-branch is stable on Travis again, and the travis build for this branch is OK. Then it can be merged again.


jan.nijtmans added on 2020-09-05 20:56:30:

> Fixed in [c045363cb0b77bf7].

Sorry, still a memory leak! Therefore I put it back in the original branch.


pooryorick added on 2020-08-31 17:24:06:

The issue this time is that ProcBodyTestProcObjCmd() synthesizes a proc body that imitates a Tcl Pro proc body and then passes it to Tcl_ProcObjCmd(), which recognizes it as a Tcl Pro compiled proc body and reuses the existing proc rather than creating a new proc. This means that two different commands now share the same Proc structure, which is odd because the Proc structure itself keeps a pointer to the Command structure it is associated with. Which command is it supposed to point to? It seems that in the case of a Tcl Pro compiled procedure, a pointer to the most recent created command is stored, and previous information discarded.

In this unusual case, the Proc structure is shared between multiple commands, but that means that Tcl_ProcObjCommand might replace the exising procPtr->cmdPtr with a new one. When it does this it should decrement the reference count of the existing cmdPtr.

Previously this wasn't an issue because reference counting of Command structures was limited to its occurrences in ByteCode instruction sequences.

Fixed in [c045363cb0b77bf7].


jan.nijtmans added on 2020-08-29 21:50:41:

Sorry, Travis fails for all "MemDebug" builds!

See: https://travis-ci.org/github/tcltk/tcl/jobs/722235077:

==== proc-4.8 TclCreateProc, procbody obj, no leak on multiple iterations FAILED
==== Contents of test case:
    set end [getbytes]
    for {set i 0} {$i < 5} {incr i} {
	procbodytest::proc tx x px
	set tmp $end
	set end [getbytes]
    }
    set leakedBytes [expr {$end - $tmp}]
---- Result was:
112
---- Result should have been (exact matching):
0
==== proc-4.8 FAILED

This indicates there's still a memory leak. Sorry, again.


jan.nijtmans added on 2020-08-28 15:20:27:

Apparently, this report is for Tcl 8.7, so added that.

This time all tests seem to pass. Congratulations! If Travis is content too, it can be merged to other branches.


pooryorick added on 2020-08-28 09:38:12:

Fixed in commit [0af4ad496cd234d3]. The problem was that the Command structure for a lambda application is not allocated, but instead synthesized by TclNRApplyObjCmd. tclProc.c/FreeLambdaInternalRep calls TclProcCleanupProc(procPtr), which tries to do cleanup on procPtr->cmdPtr. The solution is to set procPtr->cmdPtr to NULL before calling TclProcCleanupProc.


jan.nijtmans added on 2020-08-18 13:26:27:
Please look at the Travis build of this 'fix': [https://travis-ci.org/github/tcltk/tcl/builds/718869670].

I'm sorry, but this shows that the solution is still not OK.

pooryorick added on 2020-08-16 17:43:20:

A missing reference count adjustment was added in [08e3b17c955f062d].


jan.nijtmans added on 2020-08-13 12:46:42:

Uncomplete fix now reverted from core-8-branch, but bugfix branch left as-is


jan.nijtmans added on 2020-08-13 10:42:43:

Travis shows failing build on UNIX: https://travis-ci.org/github/tcltk/tcl/jobs/717502919


jan.nijtmans added on 2020-08-13 10:36:47:

When running the testsuite on MacOS (most likely on other platforms too):

aaa_exit.test
append.test
appendComp.test
Test file error: alloc: invalid block: 0x7fc0eb01f9d0: a0 ea
apply.test
Test file error: alloc: invalid block: 0x7ffdc801f9d0: d0 c7
assemble.test
assocd.test
async.test
Test file error: alloc: invalid block: 0x7f81e801f9d0: 60 e7
autoMkindex.test
....

Bisecting gives:

2020-08-12 13:35:00 9d087059ef BAD
2020-08-12 10:30:14 eab09ae8ff GOOD

Suggestion: If you are able to fix this soon, please do so. Otherwise, please revert this change from core-8-branch and continue on your branch until this is fixed.


pooryorick added on 2020-08-12 18:15:32:

Further fixes in [de751a349009f88a] and [b9d1a1ce1aef55c3].


pooryorick added on 2020-08-12 13:32:25:

Body of test case in [2da1596304896026]


pooryorick added on 2020-08-12 13:29:21:

Fixed in [37243ff47659d65c].