Tcl Source Code

The following example causes a segmentation fault:

namespace eval n {namespace ensemble create}
dict set list one ::two
namespace ensemble configure n -subcommands $list -map $list
n one

The problem is that tclEnsemble.c/BuildEnsembleConfig gets hold of a List internal representation of ensemblePtr->subcommandList, but then converts the internal representation of ensemblePtr->subcommandList to a dictionary. If it so happens that ensemblePtr->subcommandList and ensemblePtr->subcommandDict are the same Tcl_Obj, the storage for the List internal representation is deallocated.

A similar issue is [1670091].

Fix merged forward.  In reflection would have been better to fix only 8.6+.

I made another re-working and got a lot closer to what sebres contributed.

I think we're close enough to agreement now to merge the fix forward.  If we need more changes we can always make them.

Thanks!

I made another re-working and got a lot closer to what sebres contributed.

I think we're close enough to agreement now to merge the fix forward.  If we need more changes we can always make them.

Thanks!

I like the idea basically, but you've made a large performance regression on creating of each ensemble ("normal" case as well as "strange" case)...

For example in "normal" case (where map != list) by N sub-commands for ensemble: Previously (original):

  - iterate over LIST for N elements;
    - search in target hash-table (also N times).
    - search for replacement in DICT (also N times);
    - set value in target hash-table;

  - iterate over DICT for N elements;
    - iterate over LIST for N elements (N*N times);
      - search/create in target hash-table (also N*N times).
      - set value in target hash-table;
  - iterate over LIST for N elements (the second "if");
    - search in target hash-table (also N times).

E. g. by 100 sub-commands you've almost 10x overhead there:

-  23.508 µs/# 39808 # 42538.6 #/sec 935.809 nett-ms
+ 268.819 µs/#  3698 #  3720.0 #/sec 994.092 nett-ms

What exactly don't like you in my solution [f9e184a041]? Possibly goto's?

See what you think of the new fix.

https://core.tcl.tk/tcl/info/f8daa294c6a88076

I like sebres' fix because I like the idea of avoiding that additional type conversion, and I like the code as "documentation" of a real corner the case where the same value has explicitly been put to use as both a list and a dictionary.

> It does create and destroy one additional Tcl_Obj...

And converts dict to the list (and of course copy this list), then back to the dict, and then again to the list...

> And it's smaller.

Yes, the change as is 2-3 lines shorter... But are we in the save the lines mode now?

I prefer the fix in [d3ba3a9f51] . It directly addresses the need to protect the pointer returned by Tcl_ListObjGetElements() and thus directly solves the problem instead of handling one particular set of circumstances known to cause the problem.  And it's smaller.

It does create and destroy one additional Tcl_Obj, but that's been engineered to be efficient already, and TclListObjCopy() only pays a large copying cost when it needs to, which is almost never, only for bug conditions like this.

Fixed in [f9e184a041] for 8.5 without duplication, +1 additional test-case to cover the last wins case (on dict's like {one ::two one ::three}, where "one" should be mapped to "::three").

And small code review to make the things simpler.

If it is accepted I'll reintegrate it into 8.6 and higher (consider the move from tclNamesp.c to tclEnsemble.c).

> Is there a reason TclListCopy() did not figure in the solution here?

In my opinion this is anyway too hard to duplicate a list/dict in such obscure case (normally it would be enough to use construct as below to say it is in the dict instead of search in the dict):
```
if (!(i & 1) && i < subcmdc-1) {
    target = subcmdv[i+1];
} else {
    target = NULL;
}
```
But unfortunately there is another obscure case (map used quasi as filtering):
```
proc ::two {} {return 2}
proc ::three {} {return 3}
set list {one ::two one ::three}
namespace ensemble configure n -subcommands $lst -map $lst

% n one
3
```

But I've another own branch where ensembles map/list handling rewritten, and this case with duplication is fixed there. I'll try to rebase it here.

Additionally I reopen this because currently all versions (supported ensembles) are affected.
ATM fixed for core-8-branch only, I'll provide new one for 8.5 and if accepted reintegrate it for all versions (8.5 and higher).
The test case is cherry-picked as is.

er, TclListObjCopy()

Is there a reason TclListCopy() did not figure in the solution here?

Fixed in commit [046a5af026].