#5106 Tcl8.5 + incrTcl3.4 [variable] crash

obsolete: 8.5.12
closed-fixed
8
2012-09-11
2012-09-04
No

I've recently tracked down a crash that was due to faulty coding in an incrTcl methods, however, I don't know if this is a known "problem" with incrTcl, or a bug in the 8.5.8+34 combination, or just the way things are. I'm also wondering if there shouldn't be a runtime check for this condition.

So what is the problem?

itcl::class Dog {
variable bite
method bark {}
}

itcl::body Dog::bark {} {
variable bite
set bite false
puts "Bark!"
}

The crash is due to the unnecessary "variable" statement in the itcl::body. It ends up corrupting the heap. (Thanks for TCL_MEM_DEBUG!)

$ tclsh8.5
% package require Itcl
3.4
% memory validate on
% itcl::class Dog {
variable bite
method bark {}
}
% itcl::body Dog::bark {} {
variable bite
set bite false
puts "Bark!"
}
% Dog fido
fido
% fido bark
hi guard byte 0 is 0x62 b
total mallocs 114496
total frees 103026
current packets allocated 11470
current bytes allocated 482362
maximum packets allocated 15640
maximum bytes allocated 630832
high guard failed at 81ba7f8, .../tcl/unix/../generic/tclHash.c 344
8 bytes allocated at (.../incrTcl/itcl/generic/itcl_migrate.c 130)
Memory validation failure
Abort(coredump)

Discussion

  • Serg G. Brester

    Serg G. Brester - 2012-09-04

    I could not reproduce it with 8.5.11 and itcl-3-branch on windows. How often is it reproducible?

     
  • Don Porter

    Don Porter - 2012-09-04

    The failure has been present since the Tcl checkin
    http://core.tcl.tk/tcl/ci/c49c117219
    which is "modifs to help itcl adapt to VarReform"

    For many Tcl checkins prior to that, current Itcl
    source either cannot build or cannot [load].

    Appears that this bug is an indication that some
    details of adjusting to Tcl "VarReform" just never
    got done right.

     
  • Don Porter

    Don Porter - 2012-09-04

    Rolling back Itcl to the same era,
    http://core.tcl.tk/itcl/info/3f5de3c16d
    the same problem is present. This is
    not a recent Itcl regression.

     
  • Don Porter

    Don Porter - 2012-09-04

    On the bright side, whatever the problem is
    has apparently been resolved in Itcl 4.0b8.
    No troubles there.

     
  • miguel sofer

    miguel sofer - 2012-09-04

    Don: could you please check one rev back for itcl, ie [8d02855363]? That one should compile ok against 8.5 headers. I'm trying to check if it isn't something that I introduced when doing the unholy adaptation for compiling on 8.4 headers and running properly on 8.4 and 8.5

     
  • Don Porter

    Don Porter - 2012-09-05
    • priority: 5 --> 9
     
  • Don Porter

    Don Porter - 2012-09-05

    The memory corruption happens in the execution of
    the INST_VARIABLE instruction in tclExecute.c

    The "call" TclSetVarNamespaceVar(otherPtr) corrupts
    the memory. This is really a macro, and the expansion
    includes the line:

    ((VarInHash *)(otherPtr))->refCount++;

    This is actually writing one byte past allocated memory,
    so this tells me otherPtr points to a struct that is actually
    smaller in size than a VarInHash struct. I think this must
    be some Itcl declared struct that the Itcl custom resolvers
    allocate.

    I hope that's enough information for msofer to understand
    the problem at a more conceptual level and recommend a fix.

     
  • Don Porter

    Don Porter - 2012-09-05

    Looks like the root of the problem is casting a pointer to
    a Var struct to the type of VarInHash struct, and then using
    it to get at extension fields that are not there. As a guess,
    we're simply not supposed to reach this code in this scenario?
    Some guard somewhere is failing in its duty?

     
  • Donal K. Fellows

    IIRC, the 8.5 series assumes in a few key spots that all variables are either a VarInHash or a member of the LVT. In particular, it assumes that TclObjLookupVarEx(…,TCL_NAMESPACE_ONLY,…) always returns a VarInHash (or NULL, of course). It sounds like Itcl is generating them in another way. I guess we ought to check if the VAR_IN_HASHTABLE flag is set on the otherPtr (i.e., use TclIsVarInHash(otherPtr) as the guard).

    One question though: should that be combined into the guard within TclSetVarNamespaceVar? Or should it be exposed in the INST_VARIABLE code? If the latter, I think TclVariableObjCmd has the identical problem. (It should be testable by invoking [variable bite] like [set v variable;$v bite], so as to defeat compilation.)

     
  • Donal K. Fellows

    There's also the possibility that Itcl 3 might have the wrong flags set (i.e., setting the VAR_IN_HASHTABLE flag when it was never allocated as a VarInHash). If that's the case, it's got to be slated as an Itcl bug for the sake of our sanity.

     
  • Don Porter

    Don Porter - 2012-09-06

    Tcl_VariableObjCmd() suffers the same problem.

    The (Var *) returned by TclObjLookupVarEx has
    a flags field of 0.

     
  • Don Porter

    Don Porter - 2012-09-06

    Here's a demonstration of a related flaw:

    package require Itcl
    itcl::class Dog {
    method bark {}
    }
    itcl::body Dog::bark {} {
    variable bite
    }
    Dog fido
    fido bark

    can't upvar from variable to itself
    while executing
    "variable bite"
    (object "::fido" method "::Dog::bark" body line 2)
    invoked from within
    "fido bark"

    These two together indicate some ways in which
    the Itcl custom resolvers aren't quite meeting core
    expectations.

     
  • Don Porter

    Don Porter - 2012-09-06

    Branch bug-3564735 now contains a patch
    that armors Tcl against memory corruption in
    this scenario.

     
  • Don Porter

    Don Porter - 2012-09-07
    • priority: 9 --> 8
     
  • Don Porter

    Don Porter - 2012-09-11
    • status: open --> closed-fixed
     
  • Don Porter

    Don Porter - 2012-09-11

    Protection patch committed to relevant
    branches. Remaining problems are Itcl's
    and reported in the Itcl tracker.

     

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

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks