#5049 free lambda after interp -> crash

obsolete: 8.5.11
closed-fixed
9
2012-06-11
2012-06-07
Don Porter
No

% 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

Discussion

  • miguel sofer

    miguel sofer - 2012-06-07

    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.

     
  • miguel sofer

    miguel sofer - 2012-06-07
    • assigned_to: msofer --> andreas_kupries
     
  • miguel sofer

    miguel sofer - 2012-06-07

    valgrind's view of crash

     
  • Don Porter

    Don Porter - 2012-06-08

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

     
  • Don Porter

    Don Porter - 2012-06-08

    WIP on bug-3532959

     
  • Andreas Kupries

    Andreas Kupries - 2012-06-08

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

    [08:44] aku .
    [08:45] aku dgp, miguel: will try to get work on the lambda crash done during the weekend or early next week.
    [08:45] dgp nearly done
    [08:46] aku nearly done - Fixing the lambda crash ?
    [08:46] dgp yes
    [08:46] aku Oh! Thank you!
    [08:46] dgp see branch for work in progress.
    [08:47] dgp branch works for normal builds but still breaks mem access rules, so will crash in mem debug build.
    [08:48] aku Ok
    [08:49] dgp would like more ambitious revisions that eliminate the procPtr->iPtr field entirely, but lack the familiarity to do it.
    [08:49] dgp lots of lurking assumptions through here.
    [08:52] aku Very 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] aku Unclear (from looking at patch) how it breaks the mem-debug
    [08:53] dgp The crashing example is one that never stores anything in linePBodyPtr in the first place.
    [08:54] dgp Just need some sensible way to recall that, and not attempt to free what you never created.
    [08:54] aku Oh. Hm.
    [08:55] dgp so we have iPtr pointing to freed memory, iPtr->linePBodyPtr is not happy.
    [08:55] dgp though we usually don't notice that without mem debugging turned on.
    [08:56] aku Ah, I see. Guards and 0xdeadbeef
    [08:58] dgp The 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] aku Might it be safe because 'proc's cannot exist outside of the interpreter, as value, i.e. Tcl_Obj*, like lambdas ?
    [08:59] dgp It would be safe is we already prevent calling ProcCompileProc on freed interps.
    [09:00] dgp er, if we al.
    [09:01] aku Hm. 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] dgp aku, can you comment on why this data is stored in a hash table in the interp in the first place?
    [09:33] dgp If 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] aku Yes. 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] dgp ok, so a constraint is not to modify the definition of Proc ?
    [09:34] aku Yes
    [09:34] dgp ho boy
    [09:35] aku Basically, when Cisco came to us with the request for what became tip 280 they didn't want to change the binary API
    [09:35] dgp the private one.
    [09:36] aku struct Proc is private ?
    [09:36] dgp tclInt.h
    [09:37] dgp Yet another way is to keep the indirection via table strategy, but hang that table on the thread instead of the interp.
    [09:37] aku Hm. I wonder how I missed that when I wrote things.
    [09:38] dgp My guess is that it's not *really* private.
    [09:38] dgp as in someone important somewhere needs it not to change.
    [09:38] aku Right! tbcload, tclcompiler likely have access to these, making them semi-public
    [09:39] aku per-thread data ... That could work.
    [09:39] dgp Some forms of semi-public permit extension. Is this one?
    [09:40] aku ... Thinking ... I am not sure ...
    [09:40] aku With tbcload etc in mind I likely simply went all-conservative when writing the code, i.e. assuming that extension is not possible.
    [09:41] dgp Key question would be whether any third party things allocate Procs
    [09:41] dgp or if they just manipulate Procs allocated by Tcl via (Proc *) values.
    [09:42] aku I see. Well, tbcload/tclcompiler are prime candidate for such 3rd-party allocating procs
    [09:42] dgp some version of them are in SF CVS?
    [09:43] aku Some older version, yes. The basic structure should however be still good.
    [09:44] aku Would have to ask tclguy regarding stance on opening our changes/updates to these two
    [09:48] dgp Itcl 3 pokes into Proc, but only through a (Proc *)
    [10:09] dgp no luck finding tbcload/tclcompiler sources.
    [10:10] aku dbgp - The project is tclpro - http://tclpro.cvs.sourceforge.net/tclpro/
    [10:11] aku s/dbgp/dgp/
    [10:11] dgp maybe I checked out the wrong module?
    [10:12] dgp yup. When "Browse CVS" fails, I'm blind. thanks.
    [10:16] dgp ok, tclcompiler allocates the Proc struct.
    [10:19] dgp so the only possible answers in the constraints are to either have every Proc Tcl_Preserve() its interp,
    [10:20] dgp or rework so the table isn't attached to the interp so we don't care about syncing deletions.
    [10:21] jenglish dgp - ISTR struct Tcl_Interp has a little-known weak reference facility that's cheaper than Tcl_Preserve() ...
    [10:21] * jenglish checks ...
    [10:21] dgp the Handle?
    [10:21] jenglish Yeah, that's it.
    [10:21] dgp If I were free to change the Proc struct, I'd use that.
    [10:22] dgp But the Proc stores a (Tcl_Interp *), not a TclHandle.
    [10:22] jenglish Hm.
    [10:26] jenglish Are you considering Tcl_Preserve()/Tcl_Release() per procedure activation, or per procedure creation?
    [10:26] dgp not seriously, no
    [10:26] dgp Shift to thread-based table makes more sense to me.
    [10:26] jenglish That is: when you call [proc foo], or when you call [foo] itself?
    [10:26] jenglish Ah, OK.
    [10:28] jenglish Tcl_Preserve is really rather expensive, and gets more expensive the more it's used.
    [10:29] aku jenglish - Context is 3532959
    [10:30] jenglish I 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] aku IIRC Preserve/Release use lists, i.e. linear search etc.
    [10:30] jenglish Plus a mutex lock.

     
  • Andreas Kupries

    Andreas Kupries - 2012-06-08

    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.

     
  • Don Porter

    Don Porter - 2012-06-10

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

     
  • miguel sofer

    miguel sofer - 2012-06-11

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

     
  • Andreas Kupries

    Andreas Kupries - 2012-06-11

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

     
  • Don Porter

    Don Porter - 2012-06-11
    • assigned_to: andreas_kupries --> dgp
    • status: open --> closed-fixed
     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks