Tcl Source Code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Ticket UUID: 07d13d99b0a94420a5d25091b4baf917839cdce5
Title: Who broke TCL 8.6 and Tclblend ?
Type: Bug Version: 8.6.1
Submitter: anonymous Created on: 2016-04-09 22:33:39
Subsystem: 10. Objects Assigned To: jan.nijtmans
Priority: 5 Medium Severity: Severe
Status: Closed Last Modified: 2016-04-12 20:22:41
Resolution: Fixed Closed By: dgp
    Closed on: 2016-04-12 20:22:41
Description:
Tclblend (1.4.1), the popular package providing access to Java classes, no more works with Tcl 8.6.x

Tclblend was still working with Tcl 8.6.0; starting with Tcl-8.6.1, Tclblend
stopped working.

Here is a minimal crash test:
  package require java 1.4
  set jobj [java::new Object]
  $obj toString ;# CRASH


I cannot claim to understand how tclblend internally works, but I believe I've found
the way to fix it.
The fix is in the tcl-core, more specifically in "generic/tclObj.c" .
I  propose to *undo* a part of the changes occurred in 8.6.0->8.6.1 .
 (Check-in [2688d65077] 	2013-09-26 13:13:47)

Within the function "Tcl_GetCommandFromObj", change
from:
    if (SetCmdNameFromAny(interp, objPtr) != TCL_OK) {
        return NULL;
    }
to:
 if (tclCmdNameType.setFromAnyProc(interp, objPtr) != TCL_OK) {
        return NULL;
    }


Note that "by default" 
   tclCmdNameType.setFromAnyProc  ==  SetCmdNameFromAny
( i.e. the two code fragments are equivalent ).
Only some (rare) extensions do redefine
  tclCmdNameType.setFromAnyProc
and therefore it's vital that Tcl_GetCommandFromObj call the redefined function and not the 'standard' SetCmdNameFromAny.

In other words,
Tclblend redefines the function setFromAnyProc with its own "SetJavaCmdFromAny",
but this function is never called, since TCL (8.6.1) always calls the original 
SetCmdNameFromAny. This causes the crash.


I tested this change with a 'patched' Tcl-8.6.1 and Tcl-8.6.4 and I had no more crash, even with heavy applications using tclblend.

As I said before, this core-change has no impact for Tcl, since Tcl *never* changes the tclCmdNameType.setFromAnyProc value.
User Comments: dgp added on 2016-04-12 20:22:41:
I'm mostly guessing here, but if the extension needs to attach
something always recoverable to a Tcl command, I would think the
ClientData of the various Tcl_Create*Command() routines would be
a supported way to achieve that.  Imagine I'm missing some
important detail.

ferrieux added on 2016-04-12 15:43:30:
As far as I can tell, the reason is that TclBlend wants a kind of "half-shimmering" where a Java object will don the CmdType coat without entirely dropping its identity (it does it by stealing .ptr2). This is somehow related to conversion shortcuts like List<->Dict, or to Christian's patches for VecTcl so that his vector types also behave as reasonable lists without overhead.

I agree that the demand is pretty weak overall. I only felt sympathy with the idea, without a precise plan (one reason being an experiment with the Cons type years ago, bringing a super-efficient lisp-like construct for chained lists or compact pairs; at that time I felt frustrated by the footprint of the patch, which needed to touch each and every hardcoded list access).

dgp added on 2016-04-11 16:41:53:
Can we get on the record, either here or in a new ticket,
precisely why TclBlend feels the need to overwrite a field
in the tclCmdNameType ?  Is it seeking to be alerted to
every call of Tcl_GetCommandFromObj()?  For what
purpose?

What alternative alert mechanism might we offer instead?

jan.nijtmans added on 2016-04-11 07:29:57:
Ferrieux wrote:

>Though the immediate need is met, it's tempting to generalize, and look for similar 'shortcuts' where the code directly calls Set*FromAny instead of setFromAnyProc

I did that, and didn't find other places where the same problem could occur. Most built-in Tcl_ObjType's are const anyway, so it simply is impossible for extensions to change its setFromAnyProc at runtime. tclCmdNameType is an exception, and it should stay an exception for the sake of tclBlend compatibility during the whole 8.x line (IMHO).

dgp added on 2016-04-10 18:24:39:
This sort of thing ought to be way way way out of bounds.

Internal access is already a bad idea.  Internal overwrites are
beyond reasonable support demands.  They unreasonably
inhibit Tcl's own ability to improve through ongoing 
development.

Nevertheless, for important and grandfathered existing extensions
I can see some call for temporary mercy.  I can accept a limited
restoration for the remaining 8.6.* releases to give TclBlend
a temporary reprieve to find another way.

My mercy would come much easier if someone had noticed
this issue less than 3 years after it happened.  In my book,
any code that's using Tcl internals information at all has a
serious obligation to track ongoing Tcl development, take
note and report any changes that interfere with that internals
intrusion.

For 8.7+ Jan's change should be kept.

A wholesale effort to restore such changes without an actual
complaint ought to be rejected.  Only "immediate needs"
should get any consideration at all.  Ongoing core
development is not "a violation".  The violation is the
original internals intrusion and the failure to ask for
public interfaces that would end the need for them.

ferrieux added on 2016-04-10 17:37:07:
Though the immediate need is met, it's tempting to generalize, and look for similar 'shortcuts' where the code directly calls Set*FromAny instead of setFromAnyProc. Such violations of the pluggable objtype API are indeed rather numerous. Dunno how many of them are actually dangerous:

[email protected]:~/src/fos/tcl$ grep -RI 'Set[A-Za-z]*FromAny *( *[A-Za-z0-9]' . | grep -v 'int[      ]*Set[A-Za-z]*FromAny'
./generic/tclInt.h:MODULE_SCOPE int     TclSetBooleanFromAny(Tcl_Interp *interp, Tcl_Obj *objPtr);
./generic/tclExecute.c:    TclSetByteCodeFromAny(interp, objPtr, NULL, NULL);
./generic/tclExecute.c:     int result = (TclSetBooleanFromAny(NULL, valuePtr) == TCL_OK);
./generic/tclIntDecls.h:EXTERN int              TclSetByteCodeFromAny(Tcl_Interp *interp,
./generic/tclProc.c:    TclSetByteCodeFromAny(interp, bodyPtr, NULL, NULL);
./generic/tclProc.c:    result = SetLambdaFromAny(interp, lambdaPtr);
./generic/tclUtil.c:    if (SetEndOffsetFromAny(NULL, objPtr) == TCL_OK) {
./generic/tclObj.c:    } while (SetDoubleFromAny(interp, objPtr) == TCL_OK);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:           SetStringFromAny(NULL, newObjPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclStringObj.c:    SetStringFromAny(NULL, objPtr);
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:             && SetDictFromAny(interp, tmpObj) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, targetObj) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, objv[1]) != TCL_OK) {
./generic/tclDictObj.c:     && SetDictFromAny(interp, dictPtr) != TCL_OK) {
./generic/tclBinary.c:  SetByteArrayFromAny(NULL, objPtr);
./generic/tclBinary.c:  SetByteArrayFromAny(NULL, objPtr);
./generic/tclBinary.c:  SetByteArrayFromAny(NULL, objPtr);
./generic/tclNamesp.c:    if (SetNsNameFromAny(interp, objPtr) == TCL_OK) {
./generic/tclDisassemble.c:             && (TclSetByteCodeFromAny(interp, objv[2], NULL, NULL) != TCL_OK)) {
./generic/tclIO.c:    if (SetChannelFromAny(interp, objPtr) != TCL_OK) {
./generic/tclCmdMZ.c:           && (TCL_OK != TclSetBooleanFromAny(NULL, objPtr))) {
./generic/tclPathObj.c:    return SetFsPathFromAny(interp, pathPtr);
./generic/tclPathObj.c:     if (SetFsPathFromAny(interp, pathPtr) != TCL_OK) {
./generic/tclPathObj.c: if (SetFsPathFromAny(NULL, pathPtr) != TCL_OK) {
./generic/tclPathObj.c: if (SetFsPathFromAny(NULL, pathPtr) != TCL_OK) {
./generic/tclGet.c:    code = TclSetBooleanFromAny(interp, &obj);
./generic/tclInt.decls:    int TclSetByteCodeFromAny(Tcl_Interp *interp, Tcl_Obj *objPtr,
./generic/tclCompile.c:    return TclSetByteCodeFromAny(interp, objPtr, NULL, NULL);
./generic/tclListObj.c: if (SetListFromAny(interp, listPtr) != TCL_OK) {
./generic/tclListObj.c: result = SetListFromAny(interp, listPtr);
./generic/tclListObj.c: result = SetListFromAny(interp, listPtr);
./generic/tclListObj.c: result = SetListFromAny(interp, listPtr);
./generic/tclListObj.c: result = SetListFromAny(interp, listPtr);
./generic/tclListObj.c:     int result = SetListFromAny(interp, listPtr);
./generic/tclListObj.c: result = SetListFromAny(interp, listPtr);
./macosx/tclMacOSXFCmd.c:       result = SetOSTypeFromAny(interp, objPtr);

jan.nijtmans added on 2016-04-10 16:03:08:
Fixed in core-8-6-branch and trunk. Thanks for your report, and the analysis/fix

jan.nijtmans added on 2016-04-10 15:52:06:

Well, it appears I broke it in this commit: [af25d8dc8c18b1d8]