Add DamageSafetyChecker, which implements an if-all-else-fails fix for cross-mod infinite looping in attack/hurt/damage events, and changes relevant calls to attack methods to use the safety checker methods instead. Also adds damage source blacklist and compatibility warnings config options. Fixes #72.

2019-01-06 17:51:41 +00:00
parent 3bdee06738
commit f564fdce60
6 changed files with 165 additions and 12 deletions
@@ -15,6 +15,7 @@ import electroblob.wizardry.entity.EntityShield;
 import electroblob.wizardry.entity.living.ISummonedCreature;
 import electroblob.wizardry.event.SpellCastEvent;
 import electroblob.wizardry.event.SpellCastEvent.Source;
+import electroblob.wizardry.integration.DamageSafetyChecker;
 import electroblob.wizardry.packet.PacketCastContinuousSpell;
 import electroblob.wizardry.packet.PacketPlayerSync;
 import electroblob.wizardry.packet.PacketTransportation;
@@ -344,7 +345,7 @@ public class WizardData implements INBTSerializable<NBTTagCompound> {
 	 * Damages all creatures soulbound to this player by the given amount, and removes from the list any that no longer
 	 * exist.
 	 */
-	public void damageAllSoulboundCreatures(float damage){
+	public void damageAllSoulboundCreatures(float damage, String originalSourceName){

 		for(Iterator<UUID> iterator = this.soulboundCreatures.iterator(); iterator.hasNext();){

@@ -354,8 +355,8 @@ public class WizardData implements INBTSerializable<NBTTagCompound> {

 			if(entity instanceof EntityLivingBase){
 				// Retaliatory effect
-				if(entity.attackEntityFrom(MagicDamage.causeDirectMagicDamage(this.player, DamageType.MAGIC, true),
-						damage)){
+				if(DamageSafetyChecker.attackEntitySafely(entity, MagicDamage.causeDirectMagicDamage(this.player,
+						DamageType.MAGIC, true), damage, originalSourceName)){
 					// Sound only plays if the damage succeeds
 					player.playSound(SoundEvents.ENTITY_WITHER_HURT, 1.0F, player.world.rand.nextFloat() * 0.2F + 1.0F);
 				}
@@ -7,6 +7,7 @@ import electroblob.wizardry.entity.living.ISpellCaster;
 import electroblob.wizardry.entity.living.ISummonedCreature;
 import electroblob.wizardry.event.DiscoverSpellEvent;
 import electroblob.wizardry.event.SpellCastEvent;
+import electroblob.wizardry.integration.DamageSafetyChecker;
 import electroblob.wizardry.item.ItemWand;
 import electroblob.wizardry.item.ItemWizardArmour;
 import electroblob.wizardry.registry.Spells;
@@ -17,8 +18,6 @@ import electroblob.wizardry.registry.WizardryPotions;
 import electroblob.wizardry.registry.WizardrySounds;
 import electroblob.wizardry.spell.FreezingWeapon;
 import electroblob.wizardry.spell.Spell;
-import electroblob.wizardry.util.IElementalDamage;
-import electroblob.wizardry.util.MagicDamage;
 import electroblob.wizardry.util.MagicDamage.DamageType;
 import electroblob.wizardry.util.ParticleBuilder;
 import electroblob.wizardry.util.SpellModifiers;
@@ -40,6 +39,7 @@ import net.minecraft.inventory.IInventory;
 import net.minecraft.item.ItemStack;
 import net.minecraft.item.ItemSword;
 import net.minecraft.potion.PotionEffect;
+import net.minecraft.util.DamageSource;
 import net.minecraft.util.EnumHand;
 import net.minecraft.util.EnumParticleTypes;
 import net.minecraft.util.ResourceLocation;
@@ -217,9 +217,9 @@ public final class WizardryEventHandler {
 							attacker.getEntityBoundingBox().minY + attacker.height, attacker.posZ);
 				}

-				attacker.attackEntityFrom(
-						MagicDamage.causeDirectMagicDamage(event.getEntityLiving(), DamageType.SHOCK, true), 4.0f);
-				attacker.playSound(WizardrySounds.SPELL_SPARK, 1.0F, world.rand.nextFloat() * 0.4F + 1.5F);
+				DamageSafetyChecker.attackEntitySafely(attacker, MagicDamage.causeDirectMagicDamage(event.getEntityLiving(),
+						DamageType.SHOCK, true), 4.0f, event.getSource().getDamageType());
+				attacker.playSound(WizardrySounds.SPELL_STATIC_AURA_RETALIATE, 1.0F, world.rand.nextFloat() * 0.4F + 1.5F);
 			}
 		}

@@ -10,6 +10,7 @@ import com.google.common.base.Predicate;

 import electroblob.wizardry.WizardData;
 import electroblob.wizardry.Wizardry;
+import electroblob.wizardry.integration.DamageSafetyChecker;
 import electroblob.wizardry.item.ItemWand;
 import electroblob.wizardry.util.IElementalDamage;
 import electroblob.wizardry.util.IndirectMinionDamage;
@@ -383,8 +384,10 @@ public interface ISummonedCreature extends IEntityAdditionalSpawnData {

 				// For some reason Minecraft calculates knockback relative to DamageSource#getTrueSource. In vanilla this
 				// is unnoticeable, but it looks a bit weird with summoned creatures involved - so this fixes that.
-				if(WizardryUtilities.attackEntityWithoutKnockback(event.getEntity(), newSource, event.getAmount())){
-					// Using event.getSource().getTrueSource() as this means the target is knocked back from the minion
+				// Damage safety checker falls back to the original damage source, so it behaves as if the creature has
+				// no summoner.
+				if(DamageSafetyChecker.attackEntitySafely(event.getEntity(), newSource, event.getAmount(), event.getSource(), false)){
+					// Uses event.getSource().getTrueSource() as this means the target is knocked back from the minion
 					WizardryUtilities.applyStandardKnockback(event.getSource().getTrueSource(), event.getEntityLiving());
 					((ISummonedCreature)event.getSource().getTrueSource()).onSuccessfulAttack(event.getEntityLiving());
 					// If the target revenge-targeted the summoner, make it revenge-target the minion instead
@@ -0,0 +1,148 @@
+package electroblob.wizardry.integration;
+
+import com.google.common.collect.ImmutableSet;
+import electroblob.wizardry.Wizardry;
+import electroblob.wizardry.util.WizardryUtilities;
+import net.minecraft.entity.Entity;
+import net.minecraft.util.DamageSource;
+import net.minecraftforge.fml.common.Mod;
+import net.minecraftforge.fml.common.eventhandler.SubscribeEvent;
+import net.minecraftforge.fml.common.gameevent.TickEvent;
+
+import java.util.Set;
+
+/**
+ * This class implements an 'if-all-else-fails' fix for cross-mod infinite looping caused by re-applying damage in
+ * attack events (see <a href=https://github.com/Electroblob77/Wizardry/issues/72>GitHub issue #72</a> for details).
+ * The methods in this class should <b>only</b> be used when intercepting one of the attack/damage events and
+ * dealing damage from within it.
+ */
+@Mod.EventBusSubscriber
+public final class DamageSafetyChecker {
+
+	/** We don't want to tell users to add any of these to the blacklist, as that would be problematic. */
+	// NOTE: Make sure this is updated for each new version of Minecraft
+	private static final Set<String> VANILLA_DAMAGE_NAMES =  ImmutableSet.of("inFire", "lightningBolt", "onFire",
+			"lava", "hotFloor", "inWall", "cramming", "drown", "starve", "cactus", "fall", "flyIntoWall", "outOfWorld",
+			"generic", "magic", "wither", "anvil", "fallingBlock", "dragonBreath", "fireworks", "mob", "player", "arrow",
+			"thrown", "indirectMagic", "thorns", "explosion", "explosion.player");
+
+	/**
+	 * Global counter which is incremented once for each call to
+	 * {@link DamageSafetyChecker#attackEntitySafely(Entity, DamageSource, float, String, DamageSource, boolean)}
+	 * This allows for detection and avoidance of imminent StackOverflowErrors caused by looping between mods.
+	 */
+	private static int attacksThisTick = 0;
+
+	/** The number of calls per loaded entity after which damage will be reassigned. */
+	private static final int EXCESSIVE_CALL_THRESHOLD = 50;
+	/** The number of calls per loaded entity after which damage will be cancelled entirely. */
+	private static final int EXCESSIVE_CALL_LIMIT = 100;
+
+	/**
+	 * Attacks the specified target with specified damage source and damage amount, checking for the blacklist and
+	 * excessive looping in the process. Under normal circumstances, this method simply calls
+	 * {@code target.attackEntityFrom(...)}. If excessive looping is detected, the damage source is substituted for
+	 * the given fallback instead, and a warning is printed to the console.
+	 * <p></p>
+	 * This method should only be used within the attack events (LivingAttackEvent, LivingHurtEvent, LivingDamageEvent
+	 * and possibly LivingKnockBackEvent, depending on the circumstances).
+	 * @param target The target to apply the damage to.
+	 * @param source The source of the damage.
+	 * @param damage The amount of damage to be applied.
+	 * @param originalSourceName The string identifier for the original damage source (i.e. the one being replaced).
+	 *                           This allows wizardry to request that users add it to the blacklist.
+	 * @param fallback The fallback damage source for when excessive looping is detected. This <b>must not</b> be the
+	 *                 same as the re-applied source, or this method is pointless! Usually it will be more general.
+	 * @param knockback True to apply knockback as normal, false to use the knockback-free methods in WizardryUtilities.
+	 * {@link WizardryUtilities#attackEntityWithoutKnockback(Entity, DamageSource, float)}.
+	 */
+	public static boolean attackEntitySafely(Entity target, DamageSource source, float damage,
+											 String originalSourceName, DamageSource fallback, boolean knockback){
+
+		for(String sourceName : Wizardry.settings.damageSourceBlacklist){
+			if(originalSourceName.equals(sourceName)){
+				// Blacklist behaviour
+				// Same as fallback behaviour, but without the log message
+				attacksThisTick++;
+				return knockback ? target.attackEntityFrom(fallback, damage)
+						: WizardryUtilities.attackEntityWithoutKnockback(target, fallback, damage);
+			}
+		}
+
+		if(attacksThisTick > EXCESSIVE_CALL_LIMIT * target.world.loadedEntityList.size()){
+			// This should never ever happen unless another mod is intercepting non-entity-based damage and damaging
+			// the same target.
+			logInterception(originalSourceName, true);
+			return false;
+		}
+
+		if(attacksThisTick > EXCESSIVE_CALL_THRESHOLD * target.world.loadedEntityList.size()){
+			// Sometimes this is unavoidable, it's neither mod's fault but without some kind of forge standard or
+			// universal cooperation there's no easy way to prevent it.
+			logInterception(originalSourceName, false);
+			// Fallback behaviour
+			attacksThisTick++;
+			return knockback ? target.attackEntityFrom(fallback, damage)
+					: WizardryUtilities.attackEntityWithoutKnockback(target, fallback, damage);
+
+		}else{
+			// Normal behaviour
+			attacksThisTick++;
+			return knockback ? target.attackEntityFrom(source, damage)
+					: WizardryUtilities.attackEntityWithoutKnockback(target, source, damage);
+		}
+	}
+
+	/**
+	 * See {@link DamageSafetyChecker#attackEntitySafely(Entity, DamageSource, float, String, DamageSource, boolean)}.
+	 * This version is for when the original source is used as a fallback, i.e. when a damage source is being replaced
+	 * for technical reasons (e.g. summoned creatures) rather than as part of a game mechanic (e.g. shadow ward).
+	 */
+	public static boolean attackEntitySafely(Entity target, DamageSource source, float damage, DamageSource originalSource, boolean knockback){
+		return attackEntitySafely(target, source, damage, originalSource.getDamageType(), originalSource, knockback);
+	}
+
+	/**
+	 * See {@link DamageSafetyChecker#attackEntitySafely(Entity, DamageSource, float, String, DamageSource, boolean)}.
+	 * Fallback defaults to {@link DamageSource#MAGIC} and knockback defaults to true.
+	 */
+	public static boolean attackEntitySafely(Entity target, DamageSource source, float damage, String originalSourceName){
+		return attackEntitySafely(target, source, damage, originalSourceName, DamageSource.MAGIC, true);
+	}
+
+	/** Prints the appropriate message about the damage interception to the console. */
+	private static void logInterception(String originalSourceName, boolean aborted){
+
+		if(!Wizardry.settings.compatibilityWarnings) return; // No warnings if they're disabled!
+
+		boolean vanillaName = VANILLA_DAMAGE_NAMES.contains(originalSourceName);
+
+		if(aborted){
+			Wizardry.logger.warn("Entity attack excessive call limit exceeded, aborting entity damage entirely!");
+		}else{
+			Wizardry.logger.warn("Entity attack excessive call threshold exceeded, substituting for non-entity-based " +
+					"damage to avert a crash.");
+		}
+
+		if(vanillaName){
+			Wizardry.logger.info("The damage source in question had a vanilla identifier. If you know which mod may " +
+					"have caused this, consider asking the author to add a custom identifier so it may be blacklisted. " +
+					"You can turn this warning off using the compatibilityWarnings config option. Please do not report " +
+					"it to wizardry's author.");
+		}else{
+			Wizardry.logger.info("To prevent this message and improve efficiency, add \"" + originalSourceName + "\" " +
+					"(without quotes) to the damage source blacklist in the config. Please do not report " +
+					"this warning unless you have added the damage source to the blacklist already.");
+		}
+	}
+
+	@SubscribeEvent
+	public static void tick(TickEvent event){
+		// We actually want this to fire on both sides, because attacks are common code.
+		if(event.phase == TickEvent.Phase.START && event.type == TickEvent.Type.WORLD){
+			attacksThisTick = 0; // Reset the attack call counter
+		}
+	}
+
+}
@@ -67,7 +67,7 @@ public class CurseOfSoulbinding extends SpellRay {
 						&& ((IElementalDamage)event.getSource()).isRetaliatory())){
 			WizardData data = WizardData.get((EntityPlayer)event.getEntityLiving());
 			if(data != null){
-				data.damageAllSoulboundCreatures(event.getAmount());
+				data.damageAllSoulboundCreatures(event.getAmount(), event.getSource().getDamageType());
 			}
 		}
 	}
@@ -4,6 +4,7 @@ import electroblob.wizardry.constants.Element;
 import electroblob.wizardry.constants.SpellType;
 import electroblob.wizardry.constants.Tier;
 import electroblob.wizardry.item.ItemWand;
+import electroblob.wizardry.integration.DamageSafetyChecker;
 import electroblob.wizardry.util.IElementalDamage;
 import electroblob.wizardry.util.MagicDamage;
 import electroblob.wizardry.util.MagicDamage.DamageType;
@@ -62,7 +63,7 @@ public class ShadowWard extends Spell {
 				// For some reason this isn't working, so I've reverted to plain old magic damage for now.
 				//event.getEntityLiving().attackEntityFrom(
 				//		MagicDamage.causeDirectMagicDamage(event.getSource().getTrueSource(), DamageType.MAGIC, true), event.getAmount() * 0.5f);
-				event.getEntityLiving().attackEntityFrom(DamageSource.MAGIC, event.getAmount() * 0.5f);
+				DamageSafetyChecker.attackEntitySafely(event.getEntity(), DamageSource.MAGIC, event.getAmount() * 0.5f, event.getSource().getDamageType());
 				((EntityLivingBase)event.getSource().getTrueSource()).attackEntityFrom(
 						MagicDamage.causeDirectMagicDamage(event.getEntityLiving(), DamageType.MAGIC, true), event.getAmount() * 0.5f);
 			}