From e68b4918069d622ccc7f9f6f98e8432df3f3baad Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 29 Nov 2017 13:59:42 +0000 Subject: Cherry-picked test-cases from [046a5af026]: fix for issue [4f6a1ebd64]: ensemble: segmentation fault when -subcommand and -map values are the same object. --- tests/namespace.test | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/namespace.test b/tests/namespace.test index 71b6860..7d41258 100644 --- a/tests/namespace.test +++ b/tests/namespace.test @@ -1576,7 +1576,10 @@ test namespace-42.7 {ensembles: nested} { namespace delete ns set result } {{1 ::ns::x0::z} 1 2 3} -test namespace-42.8 {ensembles: [Bug 1670091]} -setup { +test namespace-42.8 { + ensembles: [Bug 1670091], panic due to pointer to a deallocated List + struct. +} -setup { proc demo args {} variable target [list [namespace which demo] x] proc trial args {variable target; string length $target} @@ -1591,6 +1594,19 @@ test namespace-42.8 {ensembles: [Bug 1670091]} -setup { rename foo {} } -result {} +test namespace-42.9 { + ensembles: [Bug 4f6a1ebd64], segmentation fault due to pointer to a + deallocated List struct. +} -setup { + namespace eval n {namespace ensemble create} + dict set list one ::two + namespace ensemble configure n -subcommands $list -map $list +} -body { + n one +} -cleanup { + namespace delete n +} -returnCodes error -match glob -result {invalid command name*} + test namespace-43.1 {ensembles: dict-driven} { namespace eval ns { namespace export x* -- cgit v0.12 From 92f8ce13dfb1299030dc844252d7478caaaa7687 Mon Sep 17 00:00:00 2001 From: dgp Date: Mon, 4 Dec 2017 18:36:14 +0000 Subject: [4f6a1ebd64] Different fix for the problem. Re-order the filling of the subcommand table so there is no longer a conflict where multiple intreps of a single value are sought. If the mapDict and exportList are the same, then each key in the mapDict is known to be an element of the exportList without needing to check. --- generic/tclNamesp.c | 115 +++++++++++++++++++++++++-------------------------- tests/namespace.test | 19 ++++++++- 2 files changed, 73 insertions(+), 61 deletions(-) diff --git a/generic/tclNamesp.c b/generic/tclNamesp.c index 4b72e03..18cd07c 100644 --- a/generic/tclNamesp.c +++ b/generic/tclNamesp.c @@ -6582,6 +6582,8 @@ BuildEnsembleConfig( int i, j, isNew; Tcl_HashTable *hash = &ensemblePtr->subcommandTable; Tcl_HashEntry *hPtr; + Tcl_Obj *mapDict = ensemblePtr->subcommandDict; + Tcl_Obj *exportList = ensemblePtr->subcmdList; if (hash->numEntries != 0) { /* @@ -6601,51 +6603,67 @@ BuildEnsembleConfig( Tcl_InitHashTable(hash, TCL_STRING_KEYS); } - /* - * See if we've got an export list. If so, we will only export exactly - * those commands, which may be either implemented by the prefix in the - * subcommandDict or mapped directly onto the namespace's commands. - */ - - if (ensemblePtr->subcmdList != NULL) { - Tcl_Obj **subcmdv, *target, *cmdObj, *cmdPrefixObj; - int subcmdc; + if (mapDict) { + /* + * We have a mapping dictionary to direct filling of the subcommand + * table. Every key, value in the dict should go into the table + * unless we have an export list that holds some of the keys back. + */ - TclListObjGetElements(NULL, ensemblePtr->subcmdList, &subcmdc, - &subcmdv); - for (i=0 ; isubcommandDict != NULL) { - Tcl_DictObjGet(NULL, ensemblePtr->subcommandDict, subcmdv[i], - &target); - if (target != NULL) { - Tcl_SetHashValue(hPtr, target); - Tcl_IncrRefCount(target); - continue; - } - } - - /* - * Not there, so map onto the namespace. Note in this case that we - * do not guarantee that the command is actually there; that is - * the programmer's responsibility (or [::unknown] of course). - */ - + /* Need to put entry in table, but it's not in mapDict */ cmdObj = Tcl_NewStringObj(ensemblePtr->nsPtr->fullName, -1); if (ensemblePtr->nsPtr->parentPtr != NULL) { Tcl_AppendStringsToObj(cmdObj, "::", name, NULL); @@ -6656,28 +6674,7 @@ BuildEnsembleConfig( Tcl_SetHashValue(hPtr, cmdPrefixObj); Tcl_IncrRefCount(cmdPrefixObj); } - } else if (ensemblePtr->subcommandDict != NULL) { - /* - * No subcmd list, but we do have a mapping dictionary so we should - * use the keys of that. Convert the dictionary's contents into the - * form required for the ensemble's internal hashtable. - */ - - Tcl_DictSearch dictSearch; - Tcl_Obj *keyObj, *valueObj; - int done; - - Tcl_DictObjFirst(NULL, ensemblePtr->subcommandDict, &dictSearch, - &keyObj, &valueObj, &done); - while (!done) { - char *name = TclGetString(keyObj); - - hPtr = Tcl_CreateHashEntry(hash, name, &isNew); - Tcl_SetHashValue(hPtr, valueObj); - Tcl_IncrRefCount(valueObj); - Tcl_DictObjNext(&dictSearch, &keyObj, &valueObj, &done); - } - } else { + } else if (mapDict == NULL) { /* * Discover what commands are actually exported by the namespace. * What we have is an array of patterns and a hash table whose keys diff --git a/tests/namespace.test b/tests/namespace.test index 7d41258..0ad8451 100644 --- a/tests/namespace.test +++ b/tests/namespace.test @@ -1599,14 +1599,29 @@ test namespace-42.9 { deallocated List struct. } -setup { namespace eval n {namespace ensemble create} - dict set list one ::two - namespace ensemble configure n -subcommands $list -map $list + set lst [dict create one ::two] + namespace ensemble configure n -subcommands $lst -map $lst } -body { n one } -cleanup { namespace delete n + unset -nocomplain lst } -returnCodes error -match glob -result {invalid command name*} +test namespace-42.10 { + ensembles: [Bug 4f6a1ebd64] segmentation fault due to pointer to a + deallocated List struct (this time with duplicate of one in "dict"). +} -setup { + namespace eval n {namespace ensemble create} + set lst [list one ::two one ::three] + namespace ensemble configure n -subcommands $lst -map $lst +} -body { + n one +} -cleanup { + namespace delete n + unset -nocomplain lst +} -returnCodes error -match glob -result {invalid command name *three*} + test namespace-43.1 {ensembles: dict-driven} { namespace eval ns { namespace export x* -- cgit v0.12 From 8a44c35e8c34860fe2ea418899c8f62fc25e06bb Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 5 Dec 2017 14:36:33 +0000 Subject: Another revised fix, much closer to sebres' patch now. --- generic/tclNamesp.c | 152 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 90 insertions(+), 62 deletions(-) diff --git a/generic/tclNamesp.c b/generic/tclNamesp.c index 18cd07c..1556ec9 100644 --- a/generic/tclNamesp.c +++ b/generic/tclNamesp.c @@ -6583,7 +6583,7 @@ BuildEnsembleConfig( Tcl_HashTable *hash = &ensemblePtr->subcommandTable; Tcl_HashEntry *hPtr; Tcl_Obj *mapDict = ensemblePtr->subcommandDict; - Tcl_Obj *exportList = ensemblePtr->subcmdList; + Tcl_Obj *subList = ensemblePtr->subcmdList; if (hash->numEntries != 0) { /* @@ -6603,78 +6603,106 @@ BuildEnsembleConfig( Tcl_InitHashTable(hash, TCL_STRING_KEYS); } - if (mapDict) { + if (subList) { + int subc; + Tcl_Obj **subv, *target, *cmdObj, *cmdPrefixObj; + char *name; + /* - * We have a mapping dictionary to direct filling of the subcommand - * table. Every key, value in the dict should go into the table - * unless we have an export list that holds some of the keys back. + * There is a list of exactly what subcommands go in the table. + * Must determine the target for each. */ - Tcl_DictSearch dictSearch; - Tcl_Obj *keyObj, *valueObj; - int done; - - Tcl_DictObjFirst(NULL, mapDict, &dictSearch, &keyObj, &valueObj, &done); - while (!done) { - int nameLen, insert = 1; - char *name = TclGetStringFromObj(keyObj, &nameLen); - - if (exportList && (exportList != mapDict)) { - Tcl_Obj **subv; - int subc; - - insert = 0; - TclListObjGetElements(NULL, exportList, &subc, &subv); - for (i = 0; i < subc; i++) { - int compareLen; - const char *compare - = TclGetStringFromObj(subv[i], &compareLen); - - if ((nameLen == compareLen) - && (memcmp(name, compare, (size_t)nameLen) == 0)) { - insert = 1; - break; - } + Tcl_ListObjGetElements(NULL, subList, &subc, &subv); + if (subList == mapDict) { + /* + * Strange case where explicit list of subcommands is same value + * as the dict mapping to targets. + */ + + for (i = 0; i < subc; i += 2) { + name = TclGetString(subv[i]); + hPtr = Tcl_CreateHashEntry(hash, name, &isNew); + if (!isNew) { + cmdObj = (Tcl_Obj *)Tcl_GetHashValue(hPtr); + Tcl_DecrRefCount(cmdObj); } - } - if (insert) { + Tcl_SetHashValue(hPtr, subv[i+1]); + Tcl_IncrRefCount(subv[i+1]); + + name = TclGetString(subv[i+1]); hPtr = Tcl_CreateHashEntry(hash, name, &isNew); - Tcl_SetHashValue(hPtr, valueObj); - Tcl_IncrRefCount(valueObj); + if (isNew) { + cmdObj = Tcl_NewStringObj(ensemblePtr->nsPtr->fullName, -1); + if (ensemblePtr->nsPtr->parentPtr != NULL) { + Tcl_AppendStringsToObj(cmdObj, "::", name, NULL); + } else { + Tcl_AppendStringsToObj(cmdObj, name, NULL); + } + cmdPrefixObj = Tcl_NewListObj(1, &cmdObj); + Tcl_SetHashValue(hPtr, cmdPrefixObj); + Tcl_IncrRefCount(cmdPrefixObj); + } } - Tcl_DictObjNext(&dictSearch, &keyObj, &valueObj, &done); - } - } - if (exportList) { - /* - * We have an export list. Put into the table each element that's - * not already there. - */ - int subc; - Tcl_Obj **subv, *cmdObj, *cmdPrefixObj; + } else { + /* Usual case where we can freely act on the list and dict. */ - TclListObjGetElements(NULL, exportList, &subc, &subv); - for (i=0 ; insPtr->fullName, -1); - if (ensemblePtr->nsPtr->parentPtr != NULL) { - Tcl_AppendStringsToObj(cmdObj, "::", name, NULL); - } else { - Tcl_AppendStringsToObj(cmdObj, name, NULL); + /* + * target was not in the dictionary so map onto the namespace. + * Note in this case that we do not guarantee that the + * command is actually there; that is the programmer's + * responsibility (or [::unknown] of course). + */ + cmdObj = Tcl_NewStringObj(ensemblePtr->nsPtr->fullName, -1); + if (ensemblePtr->nsPtr->parentPtr != NULL) { + Tcl_AppendStringsToObj(cmdObj, "::", name, NULL); + } else { + Tcl_AppendStringsToObj(cmdObj, name, NULL); + } + cmdPrefixObj = Tcl_NewListObj(1, &cmdObj); + Tcl_SetHashValue(hPtr, cmdPrefixObj); + Tcl_IncrRefCount(cmdPrefixObj); } - cmdPrefixObj = Tcl_NewListObj(1, &cmdObj); - Tcl_SetHashValue(hPtr, cmdPrefixObj); - Tcl_IncrRefCount(cmdPrefixObj); } - } else if (mapDict == NULL) { + } else if (mapDict) { + /* + * No subcmd list, but we do have a mapping dictionary so we should + * use the keys of that. Convert the dictionary's contents into the + * form required for the ensemble's internal hashtable. + */ + + Tcl_DictSearch dictSearch; + Tcl_Obj *keyObj, *valueObj; + int done; + + Tcl_DictObjFirst(NULL, ensemblePtr->subcommandDict, &dictSearch, + &keyObj, &valueObj, &done); + while (!done) { + char *name = TclGetString(keyObj); + + hPtr = Tcl_CreateHashEntry(hash, name, &isNew); + Tcl_SetHashValue(hPtr, valueObj); + Tcl_IncrRefCount(valueObj); + Tcl_DictObjNext(&dictSearch, &keyObj, &valueObj, &done); + } + } else { /* * Discover what commands are actually exported by the namespace. * What we have is an array of patterns and a hash table whose keys -- cgit v0.12 From 3eef9fcea1274b68161aa2c5ebfb6a975ac7143a Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 5 Dec 2017 15:12:05 +0000 Subject: Factor clearing of ensemble subcommand table into utility routine. --- generic/tclNamesp.c | 52 +++++++++++++++++++++++----------------------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/generic/tclNamesp.c b/generic/tclNamesp.c index 1556ec9..a2e625e 100644 --- a/generic/tclNamesp.c +++ b/generic/tclNamesp.c @@ -6480,13 +6480,31 @@ MakeCachedEnsembleCommand( */ static void +ClearTable( + EnsembleConfig *ensemblePtr) +{ + Tcl_HashTable *hash = &ensemblePtr->subcommandTable; + + if (hash->numEntries != 0) { + Tcl_HashSearch search; + Tcl_HashEntry *hPtr = Tcl_FirstHashEntry(hash, &search); + + while (hPtr != NULL) { + Tcl_Obj *prefixObj = Tcl_GetHashValue(hPtr); + Tcl_DecrRefCount(prefixObj); + hPtr = Tcl_NextHashEntry(&search); + } + ckfree((char *) ensemblePtr->subcommandArrayPtr); + } + Tcl_DeleteHashTable(hash); +} + +static void DeleteEnsembleConfig( ClientData clientData) { EnsembleConfig *ensemblePtr = clientData; Namespace *nsPtr = ensemblePtr->nsPtr; - Tcl_HashSearch search; - Tcl_HashEntry *hEnt; /* * Unlink from the ensemble chain if it has not been marked as having been @@ -6519,17 +6537,8 @@ DeleteEnsembleConfig( * Kill the pointer-containing fields. */ - if (ensemblePtr->subcommandTable.numEntries != 0) { - ckfree((char *) ensemblePtr->subcommandArrayPtr); - } - hEnt = Tcl_FirstHashEntry(&ensemblePtr->subcommandTable, &search); - while (hEnt != NULL) { - Tcl_Obj *prefixObj = Tcl_GetHashValue(hEnt); + ClearTable(ensemblePtr); - Tcl_DecrRefCount(prefixObj); - hEnt = Tcl_NextHashEntry(&search); - } - Tcl_DeleteHashTable(&ensemblePtr->subcommandTable); if (ensemblePtr->subcmdList != NULL) { Tcl_DecrRefCount(ensemblePtr->subcmdList); } @@ -6585,23 +6594,8 @@ BuildEnsembleConfig( Tcl_Obj *mapDict = ensemblePtr->subcommandDict; Tcl_Obj *subList = ensemblePtr->subcmdList; - if (hash->numEntries != 0) { - /* - * Remove pre-existing table. - */ - - Tcl_HashSearch search; - - ckfree((char *) ensemblePtr->subcommandArrayPtr); - hPtr = Tcl_FirstHashEntry(hash, &search); - while (hPtr != NULL) { - Tcl_Obj *prefixObj = Tcl_GetHashValue(hPtr); - Tcl_DecrRefCount(prefixObj); - hPtr = Tcl_NextHashEntry(&search); - } - Tcl_DeleteHashTable(hash); - Tcl_InitHashTable(hash, TCL_STRING_KEYS); - } + ClearTable(ensemblePtr); + Tcl_InitHashTable(hash, TCL_STRING_KEYS); if (subList) { int subc; -- cgit v0.12