Tcl Source Code

% set lambda x
x
% lappend lambda {set a 1}
x {set a 1}
% interp create slave
slave
% slave eval [list apply $lambda foo]
1
% interp delete slave
% unset lambda
/bin/sh: line 2: 23642 Segmentation fault      ./tclsh

allow_comments - 1

Looking at the delta. Seems good.
Doing a mem-debug build of the branch, and running the testsuite...Passes.
Approve.

Changes only involve tip280 things, so that I defer to aku. Looks ok as seen from here.

Proposed fix with test committed to bug-3532959 branch,  With
approval of aku and/or miguel, will merge to 8.5 and trunk branches.

tl;dr (Summary of chat):
We have a branch in the repository where a fix is worked on.

Discussion on why the use of pointer keyed hashtables hung
        on the Interp structure and not by extending relevant structures

        => Forbidden from changing the public API (public structures).
=> Some structures which are officially internal (in tclInt.h)
   are semi-public, including Proc.
=> User: tclcompiler, tbcload, the former actually allocates
   Proc structures on its own. (Suspected, dgp verified).

Possible change: Move the hashtables out of Interp into per-thread
data storage.

Another possibility: Preserve/Release the interp over lifetime of
the Proc structure. jenglish advises against due to O(n) in the P/R
implementation.

Discussion on the Tcler's Chat, for posterity. Summary in further comments.

[08:44]aku.
[08:45]akudgp, miguel: will try to get work on the lambda crash done during the weekend or early next week.
[08:45]dgpnearly done
[08:46]akunearly done - Fixing the lambda crash ?
[08:46]dgpyes
[08:46]akuOh! Thank you!
[08:46]dgpsee branch for work in progress.
[08:47]dgpbranch works for normal builds but still breaks mem access rules, so will crash in mem debug build.
[08:48]akuOk
[08:49]dgpwould like more ambitious revisions that eliminate the procPtr->iPtr field entirely, but lack the familiarity to do it.
[08:49]dgplots of lurking assumptions through here.
[08:52]akuVery likely. ... I believe I see how you are doing it ... During iPtr teardown you follow the trail in the hash to the proc structures and sever the connection explicitly, and in the proc teardown you check for that and bail out early
[08:53]akuUnclear (from looking at patch) how it breaks the mem-debug
[08:53]dgpThe crashing example is one that never stores anything in linePBodyPtr in the first place.
[08:54]dgpJust need some sensible way to recall that, and not attempt to free what you never created.
[08:54]akuOh. Hm.
[08:55]dgpso we have iPtr pointing to freed memory, iPtr->linePBodyPtr is not happy.
[08:55]dgpthough we usually don't notice that without mem debugging turned on.
[08:56]akuAh, I see. Guards and 0xdeadbeef
[08:58]dgpThe FindHashEntry call in ProcCompileProc has the potential to have similar trouble, but may be safe due to other crosschecks already in place for other reasons.
[08:59]akuMight it be safe because 'proc's cannot exist outside of the interpreter, as value, i.e. Tcl_Obj*, like lambdas ?
[08:59]dgpIt would be safe is we already prevent calling ProcCompileProc on freed interps.
[09:00]dgper, if we al.
[09:01]akuHm. I do not remember enough of the proc machinery to be able to say if that is true or not. I.e. not doing bcc during interp teardown
[09:32]dgpaku, can you comment on why this data is stored in a hash table in the interp in the first place?
[09:33]dgpIf we want to store what CmdFrame is associated with a Proc, why don't we just add a CmdFrame * field to the Proc struct and store it directly?  What does the indirection do for us?
[09:33]akuYes. I was not allowed to modify publicly visible structures, so I put the data into hash tables keyed by the pointers to the relevant structures
[09:34]dgpok, so a constraint is not to modify the definition of Proc ?
[09:34]akuYes
[09:34]dgpho boy
[09:35]akuBasically, when Cisco came to us with the request for what became tip 280 they didn't want to change the binary API
[09:35]dgpthe private one. 
[09:36]akustruct Proc is private ?
[09:36]dgptclInt.h
[09:37]dgpYet another way is to keep the indirection via table strategy, but hang that table on the thread instead of the interp.
[09:37]akuHm. I wonder how I missed that when I wrote things.
[09:38]dgpMy guess is that it's not *really* private.
[09:38]dgpas in someone important somewhere needs it not to change.
[09:38]akuRight! tbcload, tclcompiler likely have access to these, making them semi-public
[09:39]akuper-thread data ... That could work.
[09:39]dgpSome forms of semi-public permit extension.  Is this one?
[09:40]aku... Thinking ... I am not sure ...
[09:40]akuWith tbcload etc in mind I likely simply went all-conservative when writing the code, i.e. assuming that extension is not possible.
[09:41]dgpKey question would be whether any third party things allocate Procs
[09:41]dgpor if they just manipulate Procs allocated by Tcl via (Proc *) values.
[09:42]akuI see. Well, tbcload/tclcompiler are prime candidate for such 3rd-party allocating procs
[09:42]dgpsome version of them are in SF CVS?
[09:43]akuSome older version, yes. The basic structure should however be still good.
[09:44]akuWould have to ask tclguy regarding stance on opening our changes/updates to these two
[09:48]dgpItcl 3 pokes into Proc, but only through a (Proc *)
[10:09]dgpno luck finding tbcload/tclcompiler sources.
[10:10]akudbgp - The project is tclpro - http://tclpro.cvs.sourceforge.net/tclpro/
[10:11]akus/dbgp/dgp/
[10:11]dgpmaybe I checked out the wrong module?
[10:12]dgpyup.  When "Browse CVS" fails, I'm blind.  thanks.
[10:16]dgpok, tclcompiler allocates the Proc struct.
[10:19]dgpso the only possible answers in the constraints are to either have every Proc Tcl_Preserve() its interp,
[10:20]dgpor rework so the table isn't attached to the interp so we don't care about syncing deletions.
[10:21]jenglishdgp - ISTR struct Tcl_Interp has a little-known weak reference facility that's cheaper than Tcl_Preserve() ...
[10:21]   * jenglish checks ...
[10:21]dgpthe Handle?
[10:21]jenglishYeah, that's it.
[10:21]dgpIf I were free to change the Proc struct, I'd use that.
[10:22]dgpBut the Proc stores a (Tcl_Interp *), not a TclHandle.
[10:22]jenglishHm.
[10:26]jenglishAre you considering Tcl_Preserve()/Tcl_Release() per procedure activation, or per procedure creation?
[10:26]dgpnot seriously, no
[10:26]dgpShift to thread-based table makes more sense to me.
[10:26]jenglishThat is: when you call [proc foo], or when you call [foo] itself?
[10:26]jenglishAh, OK.
[10:28]jenglishTcl_Preserve is really rather expensive, and gets more expensive the more it's used.
[10:29]akujenglish - Context is 3532959
[10:30]jenglishI think I'd prefer to break whatever (possibly only postulated?) third-party proprietary packages that are peeking into Tcl's privates than make everybody else pay the price of proliferating Tcl_Preserve()s.
[10:30]akuIIRC Preserve/Release use lists, i.e. linear search etc.
[10:30]jenglishPlus a mutex lock.

WIP on bug-3532959

Contributing to the problem in this example
is that TclProcCleanupProc is crashing attempting
to clear away data it never stored in the first place.

File Added - 445485: err

Freeing a lambda's internal rep requires access to the interp, purely for tip-280 related data management. Not sure what to do about that, reassigning to aku.

Attaching valgrind's view of the crash on current tip of core-8-5-branch.