Fix toc/tou bug in ModulesWow64Safe

[apple1417]

ModulesWow64Safe caches seen modules to increase performance. Unfortunately, this caching contains a time of check-time of use bug when multithreading, causing spurious exceptions which seem to have no relevance to user code. This function is called from a lot of different places, so even something as innocuous as DeepPointer.Read can trigger it.

Expand for Demo

ASL Script:

state("Borderlands3") {}

init {
    new Thread(() => {
        try {
            while (true) {
                game.ModulesWow64Safe();
            }
        } catch (Exception ex) {
            print("Exception in thread: " + ex.ToString());
        }
    }).Start();
}

update {} // <-- Must exist, doesn't need to do anything

Resultant Exceptions:

00000203	285.36441040	[15192] LiveSplit.exe Error: 0 : 	
00000204	285.36444092	[15192] Exception thrown: 'System.ArgumentException' in 'update' method:	
00000205	285.36444092	[15192] An item with the same key has already been added.	
00000206	285.36444092	[15192] 	
00000207	285.36444092	[15192]    at ASL line 16 in 'update'	
00000208	285.36444092	[15192] 	
00000209	285.36444092	[15192]    at LiveSplit.ASL.ASLMethod.Call(LiveSplitState timer, ExpandoObject vars, String& version, Double& refreshRate, Object settings, ExpandoObject old, ExpandoObject current, Process game) 	
00000210	285.36444092	[15192]    at LiveSplit.ASL.ASLScript.RunMethod(ASLMethod method, LiveSplitState state, String& version) 	
00000211	285.36444092	[15192]    at LiveSplit.ASL.ASLScript.DoUpdate(LiveSplitState state) 	
00000212	285.36444092	[15192]    at LiveSplit.UI.Components.ASLComponent.UpdateScript() 	
00000213	285.39419556	[15192] LiveSplit.exe Information: 0 : 	
00000214	285.39419556	[15192] Exception in thread: System.ArgumentException: An item with the same key has already been added. 	
00000215	285.39419556	[15192]    at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource) 	
00000216	285.39419556	[15192]    at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add) 	
00000217	285.39419556	[15192]    at LiveSplit.ComponentUtil.ExtensionMethods.ModulesWow64Safe(Process p) 	
00000218	285.39419556	[15192]    at CompiledScript.<>c__DisplayClass1.<Execute>b__0() in c:\Users\apple1417\AppData\Local\Temp\1etpk5uq.0.cs:line 31 	

The bug comes from this part of the code:

            int hash = p.StartTime.GetHashCode() + p.Id + (int)numMods;
            if (ModuleCache.ContainsKey(hash))
                return ModuleCache[hash];
            ...
            ModuleCache.Add(hash, ret.ToArray());

This is just a classic toc/tou, the cache does not contain the key while both threads are passing that call, meaning both continue and then try to add it later, so Dictionary.Add throws the ArgumentException due to the key already existing.

When writing a component it's pretty trivial to trigger the exception. When writing asls, this function is called at the start of every method, before any user code, so you might assume this means everything will be cached before the user can start a thread to break things. However, if the attached process loads extra modules after the init call (which setup the thread), the hash will change, and both threads will try load all the module information again (as demonstrated).

Using index assignment instead will avoid the exception, it'll just overwrite. This should be safe because it should always overwrite with the exact same data - it can't miss a module cause the number of modules affects the hash, and if any winapi functions fail it throws before ever getting to the assignment.

[just-ero]

You could consider using lock:

lock (ModuleCache)
{
    ...
}