Don't leak static proplists with cyclic references

If a local variable in a definition was set to a proplist inside the
Definition() callback, and that proplist contained cyclic references
then those references were leaked. Typically cyclic references for
script-created proplists are broken in
C4PropListScript::ClearScriptPropLists, however definition proplists
are changed to be static proplists in
C4PropList::FreezeAndMakeStaticRecursively.

To fix this, each script host maintains a list of proplists made static
by FreezeAndMakeStaticRecursively, and explicitly deletes all of these
proplists on Clear().

This leak also leads to an assertion failure inside
C4PropListScript::ClearScriptPropLists in debug mode, and can also be
observed by C4PropList::PropLists not being empty after game clear.

The definition in Objects.ocd/Helpers.ocd/UserAction.ocd constructs
cyclic proplists in its Definition() call. A simpler, more minimal way
to provoke the leak is the following (it provokes the leak but not the
assertion failure):

    local bla;

    func Definition(def)
    {
        bla = {};
        bla.test = { Name="Test222" , Options = { Name="Test333" } };
        bla.test.Options.Link = { Name="Test444", Blub=bla.test };
    }
console-destruction
Armin Burgmeier 2016-09-30 17:25:28 -10:00
parent 732fff3029
commit 836927d93c
7 changed files with 52 additions and 7 deletions

View File

@ -86,6 +86,18 @@ void C4AulScriptEngine::Clear()
GlobalNamed.SetNameList(&GlobalNamedNames);
delete pGlobalEffects; pGlobalEffects=NULL;
UserFiles.clear();
// Delete all global proplists made static (breaks
// cyclic references).
for (C4Value& value: OwnedPropLists)
{
C4PropList* plist = value.getPropList();
if (plist)
{
if (plist->Delete()) delete plist;
else plist->Clear();
}
}
OwnedPropLists.clear();
}
void C4AulScriptEngine::RegisterGlobalConstant(const char *szName, const C4Value &rValue)

View File

@ -109,6 +109,7 @@ protected:
// all open user files
// user files aren't saved - they are just open temporary e.g. during game saving
std::list<C4AulUserFile> UserFiles;
std::vector<C4Value> OwnedPropLists;
public:
int warnCnt, errCnt; // number of warnings/errors

View File

@ -136,6 +136,9 @@ void C4ScriptHost::UnLink()
p->SetProperty(P_Prototype, C4VPropList(Engine->GetPropList()));
}
// Delete cyclic references of owned proplists
DeleteOwnedPropLists();
// includes will have to be re-resolved now
IncludesResolved = false;
@ -179,8 +182,9 @@ void C4AulScriptEngine::Link(C4DefList *rDefs)
// Done modifying the proplists now
for (C4ScriptHost *s = Child0; s; s = s->Next)
s->GetPropList()->FreezeAndMakeStaticRecursively();
GetPropList()->FreezeAndMakeStaticRecursively();
s->GetPropList()->FreezeAndMakeStaticRecursively(&s->ownedPropLists);
GetPropList()->FreezeAndMakeStaticRecursively(&OwnedPropLists);
}
catch (C4AulError &err)
{

View File

@ -283,7 +283,7 @@ C4PropList::C4PropList(C4PropList * prototype):
#endif
}
C4PropListStatic *C4PropList::FreezeAndMakeStaticRecursively(const C4PropListStatic *parent, C4String * key)
C4PropListStatic *C4PropList::FreezeAndMakeStaticRecursively(std::vector<C4Value>* prop_lists, const C4PropListStatic *parent, C4String * key)
{
Freeze();
// Already static?
@ -301,6 +301,9 @@ C4PropListStatic *C4PropList::FreezeAndMakeStaticRecursively(const C4PropListSta
if (ref == &holder) ref = ref->NextRef;
ref->SetPropList(this_static);
}
// store reference
if (prop_lists)
prop_lists->push_back(C4VPropList(this_static));
// "this" should be deleted as holder goes out of scope
}
// Iterate over sorted list of elements to make static
@ -318,7 +321,7 @@ C4PropListStatic *C4PropList::FreezeAndMakeStaticRecursively(const C4PropListSta
C4PropListStatic *child_static = child_proplist->IsStatic();
if (!child_static || (child_static->GetParent() == this_static && child_static->GetParentKeyName() == prop_name))
{
child_proplist->FreezeAndMakeStaticRecursively(this_static, prop_name);
child_proplist->FreezeAndMakeStaticRecursively(prop_lists, this_static, prop_name);
}
}
}

View File

@ -133,7 +133,8 @@ public:
// Freeze this and all proplist in properties and ensure they are static proplists
// If a proplist is not static, replace it with a static proplist and replace all instances
C4PropListStatic *FreezeAndMakeStaticRecursively(const C4PropListStatic *parent = nullptr, C4String * key = nullptr);
// Place references to all proplists made static in the given value array
C4PropListStatic *FreezeAndMakeStaticRecursively(std::vector<C4Value>* prop_lists, const C4PropListStatic *parent = nullptr, C4String * key = nullptr);
virtual void Denumerate(C4ValueNumbers *);
virtual ~C4PropList();

View File

@ -53,6 +53,7 @@ void C4ScriptHost::Clear()
C4ComponentHost::Clear();
Script.Clear();
LocalValues.Clear();
DeleteOwnedPropLists();
SourceScripts.clear();
SourceScripts.push_back(this);
if (stringTable)
@ -67,6 +68,23 @@ void C4ScriptHost::Clear()
State = ASS_NONE;
}
void C4ScriptHost::DeleteOwnedPropLists()
{
// delete all static proplists associated to this script host.
// Note that just clearing the vector is not enough in case of
// cyclic references.
for (C4Value& value: ownedPropLists)
{
C4PropList* plist = value.getPropList();
if (plist)
{
if (plist->Delete()) delete plist;
else plist->Clear();
}
}
ownedPropLists.clear();
}
void C4ScriptHost::UnlinkOwnedFunctions()
{
// Remove owned functions from their parents. This solves a problem

View File

@ -53,8 +53,6 @@ public:
std::list<C4ScriptHost *> SourceScripts;
StdCopyStrBuf ScriptName; // script name
void UnlinkOwnedFunctions();
protected:
C4ScriptHost();
void Unreg(); // remove from list
@ -65,6 +63,9 @@ protected:
virtual bool Parse(); // parse preparsed script; return if successfull
virtual void UnLink(); // reset to unlinked state
void UnlinkOwnedFunctions();
void DeleteOwnedPropLists();
void Warn(const char *pMsg, ...) GNUC_FORMAT_ATTRIBUTE_O;
C4AulScriptEngine *Engine; //owning engine
@ -89,6 +90,11 @@ protected:
// list of all functions generated from code in this script host
std::set<C4AulScriptFunc*> ownedFunctions;
// list of all static proplists that refer to this script host
// filled in at link time and used to delete all proplists
// in Clear() even in case of cyclic references.
std::vector<C4Value> ownedPropLists;
friend class C4AulParse;
friend class C4AulProfiler;
friend class C4AulScriptEngine;