Tcl Source Code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Ticket UUID: 4f6a1ebd64cffa3ef795d83cb15d92c40cfb12bd
Title: ensemble: segmentation fault when -subcommand and -map values are the same object
Type: Bug Version: >= 8.5
Submitter: pooryorick Created on: 2017-11-25 15:41:13
Subsystem: 21. [namespace] Assigned To: dgp
Priority: 5 Medium Severity: Important
Status: Closed Last Modified: 2017-12-05 17:25:20
Resolution: Fixed Closed By: dgp
    Closed on: 2017-12-05 17:25:20
Description:

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].

User Comments: dgp added on 2017-12-05 17:25:20:
Fix merged forward.  In reflection would have been better to fix only 8.6+.

dgp added on 2017-12-05 14:45:27:
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!

dgp added on 2017-12-05 14:45:11:
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!

sebres added on 2017-12-04 21:14:07:

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;
New ([f8daa294c6a88076]):
  - 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
Therefore I don't think it would be better now...

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


dgp added on 2017-12-04 18:37:42:
See what you think of the new fix.

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

pooryorick added on 2017-12-04 10:18:42:

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.


sebres added on 2017-11-30 18:57:19:
> 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?

dgp added on 2017-11-30 18:18:57:
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.

sebres added on 2017-11-29 14:18:35:

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).


sebres added on 2017-11-28 21:28:23:
> 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.

dgp added on 2017-11-28 18:19:52:
er, TclListObjCopy()

dgp added on 2017-11-28 18:19:27:
Is there a reason TclListCopy() did not figure in the solution here?

pooryorick added on 2017-11-25 16:41:36:

Fixed in commit [046a5af026].