From b451a5a67259a38ddb10beb85d740b98488ec3bb Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Fri, 19 Apr 2013 21:34:34 +0200 Subject: [PATCH] Adding a plugin verification system detecting common programmer errors Initially we will detect plugins that attempt to register a listener in ProtocolLib without setting "depend" or "soft-depend". --- .../comphenix/protocol/ProtocolLibrary.java | 2 +- .../injector/PacketFilterManager.java | 30 ++- .../protocol/injector/PluginVerifier.java | 211 ++++++++++++++++++ 3 files changed, 239 insertions(+), 4 deletions(-) create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/injector/PluginVerifier.java diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index d1e27b6f..3071707f 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -147,7 +147,7 @@ public class ProtocolLibrary extends JavaPlugin { unhookTask = new DelayedSingleTask(this); protocolManager = new PacketFilterManager( - getClassLoader(), getServer(), version, unhookTask, detailedReporter); + getClassLoader(), getServer(), this, version, unhookTask, detailedReporter); // Setup error reporter detailedReporter.addGlobalParameter("manager", protocolManager); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java index af7453cc..6aa6300b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -148,17 +148,22 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok // Spigot listener, if in use private SpigotPacketInjector spigotInjector; + // Plugin verifier + private PluginVerifier pluginVerifier; + /** * Only create instances of this class if protocol lib is disabled. */ - public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, ErrorReporter reporter) { - this(classLoader, server, new MinecraftVersion(server), unhookTask, reporter); + public PacketFilterManager(ClassLoader classLoader, Server server, Plugin library, DelayedSingleTask unhookTask, ErrorReporter reporter) { + this(classLoader, server, library, new MinecraftVersion(server), unhookTask, reporter); } /** * Only create instances of this class if protocol lib is disabled. */ - public PacketFilterManager(ClassLoader classLoader, Server server, MinecraftVersion mcVersion, DelayedSingleTask unhookTask, ErrorReporter reporter) { + public PacketFilterManager(ClassLoader classLoader, Server server, Plugin library, + MinecraftVersion mcVersion, DelayedSingleTask unhookTask, ErrorReporter reporter) { + if (reporter == null) throw new IllegalArgumentException("reporter cannot be NULL."); if (classLoader == null) @@ -177,6 +182,9 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok this.classLoader = classLoader; this.reporter = reporter; + // The plugin verifier + this.pluginVerifier = new PluginVerifier(library); + // Used to determine if injection is needed Predicate isInjectionNecessary = new Predicate() { @Override @@ -267,6 +275,20 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok return ImmutableSet.copyOf(packetListeners); } + /** + * Warn of common programming mistakes. + * @param plugin - plugin to check. + */ + private void printPluginWarnings(Plugin plugin) { + switch (pluginVerifier.verify(plugin)) { + case NO_DEPEND: + reporter.reportWarning(this, plugin + " doesn't depend on ProtocolLib. Check that its plugin.yml has a 'depend' directive."); + case VALID: + // Do nothing + break; + } + } + @Override public void addPacketListener(PacketListener listener) { if (listener == null) @@ -275,6 +297,8 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok // A listener can only be added once if (packetListeners.contains(listener)) return; + // Check plugin + printPluginWarnings(listener.getPlugin()); ListeningWhitelist sending = listener.getSendingWhitelist(); ListeningWhitelist receiving = listener.getReceivingWhitelist(); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PluginVerifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PluginVerifier.java new file mode 100644 index 00000000..9be75b1b --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PluginVerifier.java @@ -0,0 +1,211 @@ +package com.comphenix.protocol.injector; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import org.bukkit.Bukkit; +import org.bukkit.plugin.Plugin; +import org.bukkit.plugin.PluginLoadOrder; + +import com.google.common.collect.Sets; + +/** + * Determine if a plugin using ProtocolLib is correct. + * + * @author Kristian + */ +class PluginVerifier { + /** + * A named plugin cannot be found. + * @author Kristian + */ + public static class PluginNotFoundException extends RuntimeException { + /** + * Generated by Eclipse. + */ + private static final long serialVersionUID = 8956699101336877611L; + + public PluginNotFoundException() { + super(); + } + + public PluginNotFoundException(String message) { + super(message); + } + } + + public enum VerificationResult { + VALID, + + /** + * The plugin doesn't depend on ProtocolLib directly or indirectly. + */ + NO_DEPEND; + + /** + * Determine if the verification was valid. + */ + public boolean isValid() { + return this == VALID; + } + } + + /** + * Set of plugins that have been loaded after ProtocolLib. + */ + private final Set loadedAfter = new HashSet(); + + /** + * Reference to ProtocolLib. + */ + private final Plugin dependency; + + /** + * Construct a new plugin verifier. + * @param dependency - reference to ProtocolLib, a dependency we require of plugins. + */ + public PluginVerifier(Plugin dependency) { + if (dependency == null) + throw new IllegalArgumentException("dependency cannot be NULL."); + // This would screw with the assumption in hasDependency(Plugin, Plugin) + if (safeConversion(dependency.getDescription().getLoadBefore()).size() > 0) + throw new IllegalArgumentException("dependency cannot have a load directives."); + + this.dependency = dependency; + } + + /** + * Retrieve a plugin by name. + * @param pluginName - the non-null name of the plugin to retrieve. + * @return The retrieved plugin. + * @throws PluginNotFoundException If a plugin with the given name cannot be found. + */ + private Plugin getPlugin(String pluginName) { + Plugin plugin = Bukkit.getPluginManager().getPlugin(pluginName); + + // Ensure that the plugin exists + if (plugin != null) + return plugin; + else + throw new PluginNotFoundException("Cannot find plugin " + pluginName); + } + + /** + * Performs simple verifications on the given plugin. + *

+ * Results may be cached. + * @param pluginName - the plugin to verify. + * @return A verification result. + * @throws IllegalArgumentException If plugin name is NULL. + * @throws PluginNotFoundException If a plugin with the given name cannot be found. + */ + public VerificationResult verify(String pluginName) { + if (pluginName == null) + throw new IllegalArgumentException("pluginName cannot be NULL."); + return verify(getPlugin(pluginName)); + } + + /** + * Performs simple verifications on the given plugin. + *

+ * Results may be cached. + * @param plugin - the plugin to verify. + * @return A verification result. + * @throws IllegalArgumentException If plugin name is NULL. + * @throws PluginNotFoundException If a plugin with the given name cannot be found. + */ + public VerificationResult verify(Plugin plugin) { + if (plugin == null) + throw new IllegalArgumentException("plugin cannot be NULL."); + + if (!loadedAfter.contains(plugin.getName())) { + if (verifyLoadOrder(dependency, plugin)) { + // Memorize + loadedAfter.add(plugin.getName()); + } else { + return VerificationResult.NO_DEPEND; + } + } + + // Everything seems to be in order + return VerificationResult.VALID; + } + + /** + * Determine if a given plugin is guarenteed to be loaded before the other. + *

+ * Note that the before plugin is assumed to have no "load" directives - that is, plugins to be + * loaded after itself. The after plugin may have "load" directives, but it is irrelevant for our purposes. + * @param beforePlugin - the plugin that is loaded first. + * @param afterPlugin - the plugin that is loaded last. + * @return TRUE if it will, FALSE if it may or must load in the opposite other. + */ + private boolean verifyLoadOrder(Plugin beforePlugin, Plugin afterPlugin) { + // If a plugin has a dependency, it will be loaded after its dependency + if (hasDependency(afterPlugin, beforePlugin)) { + return true; + } + + // No dependency - check the load order + if (beforePlugin.getDescription().getLoad() == PluginLoadOrder.STARTUP && + afterPlugin.getDescription().getLoad() == PluginLoadOrder.POSTWORLD) { + return true; + } + return false; + } + + /** + * Determine if a plugin has a given dependency, either directly or indirectly. + * @param plugin - the plugin to check. + * @param dependency - the dependency to find. + * @return TRUE if the plugin has the given dependency, FALSE otherwise. + */ + private boolean hasDependency(Plugin plugin, Plugin dependency) { + return hasDependency(plugin, dependency, Sets.newHashSet()); + } + + /** + * Convert a list to a set. + *

+ * A null list will be converted to an empty set. + * @param list - the list to convert. + * @return The converted list. + */ + private Set safeConversion(List list) { + if (list == null) + return Collections.emptySet(); + else + return Sets.newHashSet(list); + } + + // Avoid cycles + private boolean hasDependency(Plugin plugin, Plugin dependency, Set checked) { + Set childNames = Sets.union( + safeConversion(plugin.getDescription().getDepend()), + safeConversion(plugin.getDescription().getSoftDepend()) + ); + + // Ensure that the same plugin isn't processed twice + if (!checked.add(plugin.getName())) { + throw new IllegalStateException("Cycle detected in dependency graph: " + plugin); + } + // Look for the dependency in the immediate children + if (childNames.contains(dependency.getName())) { + return true; + } + + // Recurse through their dependencies + for (String childName : childNames) { + Plugin childPlugin = getPlugin(childName); + + if (hasDependency(childPlugin, dependency, checked)) { + return true; + } + } + + // No dependency found! + return false; + } +}