From f564fdce60043a4561d0b958dbb2a206a72b8b7c Mon Sep 17 00:00:00 2001 From: Electroblob77 <35599699+Electroblob77@users.noreply.github.com> Date: Sun, 6 Jan 2019 17:51:41 +0000 Subject: [PATCH] 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. --- .../java/electroblob/wizardry/WizardData.java | 7 +- .../wizardry/WizardryEventHandler.java | 10 +- .../entity/living/ISummonedCreature.java | 7 +- .../integration/DamageSafetyChecker.java | 148 ++++++++++++++++++ .../wizardry/spell/CurseOfSoulbinding.java | 2 +- .../wizardry/spell/ShadowWard.java | 3 +- 6 files changed, 165 insertions(+), 12 deletions(-) create mode 100644 src/main/java/electroblob/wizardry/integration/DamageSafetyChecker.java diff --git a/src/main/java/electroblob/wizardry/WizardData.java b/src/main/java/electroblob/wizardry/WizardData.java index 1ca6f9e7..fd27d9c0 100644 --- a/src/main/java/electroblob/wizardry/WizardData.java +++ b/src/main/java/electroblob/wizardry/WizardData.java @@ -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 { * 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 iterator = this.soulboundCreatures.iterator(); iterator.hasNext();){ @@ -354,8 +355,8 @@ public class WizardData implements INBTSerializable { 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); } diff --git a/src/main/java/electroblob/wizardry/WizardryEventHandler.java b/src/main/java/electroblob/wizardry/WizardryEventHandler.java index 32da4e75..ebebcb77 100644 --- a/src/main/java/electroblob/wizardry/WizardryEventHandler.java +++ b/src/main/java/electroblob/wizardry/WizardryEventHandler.java @@ -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); } } diff --git a/src/main/java/electroblob/wizardry/entity/living/ISummonedCreature.java b/src/main/java/electroblob/wizardry/entity/living/ISummonedCreature.java index b510dc9c..5e27cb57 100644 --- a/src/main/java/electroblob/wizardry/entity/living/ISummonedCreature.java +++ b/src/main/java/electroblob/wizardry/entity/living/ISummonedCreature.java @@ -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 diff --git a/src/main/java/electroblob/wizardry/integration/DamageSafetyChecker.java b/src/main/java/electroblob/wizardry/integration/DamageSafetyChecker.java new file mode 100644 index 00000000..5e3dfefd --- /dev/null +++ b/src/main/java/electroblob/wizardry/integration/DamageSafetyChecker.java @@ -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 GitHub issue #72 for details). + * The methods in this class should only 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 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. + *

+ * 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 must not 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 + } + } + +} diff --git a/src/main/java/electroblob/wizardry/spell/CurseOfSoulbinding.java b/src/main/java/electroblob/wizardry/spell/CurseOfSoulbinding.java index d6e73fb2..76797c44 100644 --- a/src/main/java/electroblob/wizardry/spell/CurseOfSoulbinding.java +++ b/src/main/java/electroblob/wizardry/spell/CurseOfSoulbinding.java @@ -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()); } } } diff --git a/src/main/java/electroblob/wizardry/spell/ShadowWard.java b/src/main/java/electroblob/wizardry/spell/ShadowWard.java index 6c1db4c4..7d2736ea 100644 --- a/src/main/java/electroblob/wizardry/spell/ShadowWard.java +++ b/src/main/java/electroblob/wizardry/spell/ShadowWard.java @@ -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); }