From f61b52ccae9ade81c6b2b1e4bc6bd062c9182e59 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:12:41 +1300 Subject: [PATCH 1/6] editor: Fix a benign loop-variable-inside-closure issue The getter is accessed exactly once, at call time. The setter is stored, and invoked later on losing focus, but we pass a bogus one anyway, so there is no issue. --- OpenRA.Editor/Form1.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/OpenRA.Editor/Form1.cs b/OpenRA.Editor/Form1.cs index 6364e5dd2b..c9834e1c8d 100644 --- a/OpenRA.Editor/Form1.cs +++ b/OpenRA.Editor/Form1.cs @@ -86,14 +86,17 @@ namespace OpenRA.Editor // TODO: make this work properly foreach (var init in Program.Rules.Actors[kv.Value.Type].GetInitKeys()) - apd.AddRow(init.First, + { + var initName = init.First; + apd.AddRow(initName, apd.MakeEditorControl(init.Second, () => { var nodesDict = objSaved.ToDictionary(); - return nodesDict.ContainsKey(init.First) ? nodesDict[init.First].Value : null; + return nodesDict.ContainsKey(initName) ? nodesDict[initName].Value : null; }, _ => { })); + } apd.ShowDialog(); From edbd65bdf368b288c27e1a4be3effe5c11980ccd Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:17:09 +1300 Subject: [PATCH 2/6] filesystem: Fix benign use of loop variable in closure This closure never outlives a single loop iteration. --- OpenRA.Game/FileSystem/GlobalFileSystem.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/OpenRA.Game/FileSystem/GlobalFileSystem.cs b/OpenRA.Game/FileSystem/GlobalFileSystem.cs index 8e45075217..ec7ced255a 100644 --- a/OpenRA.Game/FileSystem/GlobalFileSystem.cs +++ b/OpenRA.Game/FileSystem/GlobalFileSystem.cs @@ -199,13 +199,14 @@ namespace OpenRA.FileSystem foreach (var ext in exts) { + var possibleName = filename + ext; var folder = MountedFolders - .Where(x => x.Exists(filename + ext)) + .Where(x => x.Exists(possibleName)) .MaxByOrDefault(x => x.Priority); if (folder != null) { - s = folder.GetContent(filename + ext); + s = folder.GetContent(possibleName); return true; } } From f2492f41554284ca4c95a6ddb713be4225f2e2b7 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:19:55 +1300 Subject: [PATCH 3/6] LevelUpCrateAction: fix loop variable in closure bug This was a real bug -- if there were multiple recipients, we'd apply the levels N times to the last recipient rather than once to each. --- OpenRA.Mods.RA/Crates/LevelUpCrateAction.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/OpenRA.Mods.RA/Crates/LevelUpCrateAction.cs b/OpenRA.Mods.RA/Crates/LevelUpCrateAction.cs index 50d03a2e74..5212e408d2 100644 --- a/OpenRA.Mods.RA/Crates/LevelUpCrateAction.cs +++ b/OpenRA.Mods.RA/Crates/LevelUpCrateAction.cs @@ -68,9 +68,10 @@ namespace OpenRA.Mods.RA foreach (var actor in inRange.Append(collector)) { - actor.World.AddFrameEndTask(w => + var recipient = actor; // loop variable in closure hazard + recipient.World.AddFrameEndTask(w => { - var gainsExperience = actor.TraitOrDefault(); + var gainsExperience = recipient.TraitOrDefault(); if (gainsExperience != null) gainsExperience.GiveLevels(((LevelUpCrateActionInfo)info).Levels); }); From 98563eb854974d210ba69e0dbd2a320bb354bdb5 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:24:00 +1300 Subject: [PATCH 4/6] PrimaryBuilding: Fix benign loop variable in closure hazard This never outlived a single loop iteration. --- OpenRA.Mods.RA/PrimaryBuilding.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/OpenRA.Mods.RA/PrimaryBuilding.cs b/OpenRA.Mods.RA/PrimaryBuilding.cs index 6cf53664a3..226f1ff6fe 100755 --- a/OpenRA.Mods.RA/PrimaryBuilding.cs +++ b/OpenRA.Mods.RA/PrimaryBuilding.cs @@ -67,13 +67,16 @@ namespace OpenRA.Mods.RA // TODO: THIS IS SHIT // Cancel existing primaries foreach (var p in self.Info.Traits.Get().Produces) + { + var productionType = p; // benign closure hazard foreach (var b in self.World .ActorsWithTrait() .Where(a => a.Actor.Owner == self.Owner && a.Trait.IsPrimary && - a.Actor.Info.Traits.Get().Produces.Contains(p))) + a.Actor.Info.Traits.Get().Produces.Contains(productionType))) b.Trait.SetPrimaryProducer(b.Actor, false); + } isPrimary = true; From 3360b1196239b9ca237984fd1e07a145139e2181 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:27:31 +1300 Subject: [PATCH 5/6] WithMuzzleFlash: fix loop variable in closure hazard This fixes a real bug -- if the actor had multiple armaments, we'd end up placing the muzzle flashes all based on the offsets of the last one. --- OpenRA.Mods.RA/Render/WithMuzzleFlash.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/OpenRA.Mods.RA/Render/WithMuzzleFlash.cs b/OpenRA.Mods.RA/Render/WithMuzzleFlash.cs index 0621331604..c5d7e4e028 100644 --- a/OpenRA.Mods.RA/Render/WithMuzzleFlash.cs +++ b/OpenRA.Mods.RA/Render/WithMuzzleFlash.cs @@ -38,6 +38,8 @@ namespace OpenRA.Mods.RA.Render foreach (var arm in self.TraitsImplementing()) { + var armClosure = arm; // closure hazard in AnimationWithOffset + // Skip armaments that don't define muzzles if (arm.Info.MuzzleSequence == null) continue; @@ -55,7 +57,7 @@ namespace OpenRA.Mods.RA.Render visible.Add(barrel, false); anims.Add(barrel, new AnimationWithOffset(muzzleFlash, - () => info.IgnoreOffset ? WVec.Zero : arm.MuzzleOffset(self, barrel), + () => info.IgnoreOffset ? WVec.Zero : armClosure.MuzzleOffset(self, barrel), () => !visible[barrel], () => false, p => WithTurret.ZOffsetFromCenter(self, p, 2))); From ea475ed39b2d34bf465065f493ff2bdbfd985bfe Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 7 Oct 2014 18:30:23 +1300 Subject: [PATCH 6/6] DomainIndex: Fix benign loop variable in closure hazard This never outlived a single iteration of the neighbors loop. --- OpenRA.Mods.RA/World/DomainIndex.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OpenRA.Mods.RA/World/DomainIndex.cs b/OpenRA.Mods.RA/World/DomainIndex.cs index 9b6704cca2..4e27de07c6 100644 --- a/OpenRA.Mods.RA/World/DomainIndex.cs +++ b/OpenRA.Mods.RA/World/DomainIndex.cs @@ -89,7 +89,8 @@ namespace OpenRA.Mods.RA foreach (var cell in dirtyCells) { // Select all neighbors inside the map boundries - var neighbors = CVec.directions.Select(d => d + cell) + var thisCell = cell; // benign closure hazard + var neighbors = CVec.directions.Select(d => d + thisCell) .Where(c => map.Contains(c)); var found = false;