Skip to content

Commit

Permalink
Handle exceptions in injected code.
Browse files Browse the repository at this point in the history 
This is very important, otherwise the server may crash unnecessarily.
  • Loading branch information
@aadnk
aadnk committed Oct 29, 2012
1 parent f9c0f21 commit 0dc2bfe
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 81 deletions.
5 changes: 2 additions & 3 deletions ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import com.comphenix.protocol.ProtocolManager;
import com.comphenix.protocol.async.AsyncFilterManager;
import com.comphenix.protocol.async.AsyncMarker;
import com.comphenix.protocol.error.DetailedErrorReporter;
import com.comphenix.protocol.error.ErrorReporter;
import com.comphenix.protocol.events.*;
import com.comphenix.protocol.injector.player.PlayerInjectionHandler;
Expand Down Expand Up @@ -142,7 +141,7 @@ public enum PlayerInjectHooks {
* Only create instances of this class if protocol lib is disabled.
* @param unhookTask
*/
public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, DetailedErrorReporter reporter) {
public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, ErrorReporter reporter) {
if (reporter == null)
throw new IllegalArgumentException("reporter cannot be NULL.");
if (classLoader == null)
Expand Down Expand Up @@ -175,7 +174,7 @@ public boolean apply(@Nullable GamePhase phase) {
try {
// Initialize injection mangers
this.playerInjection = new PlayerInjectionHandler(classLoader, reporter, isInjectionNecessary, this, packetListeners, server);
this.packetInjector = new PacketInjector(classLoader, this, playerInjection);
this.packetInjector = new PacketInjector(classLoader, this, playerInjection, reporter);
this.asyncFilterManager = new AsyncFilterManager(reporter, server.getScheduler(), this);

// Attempt to load the list of server and client packets
Expand Down
9 changes: 7 additions & 2 deletions ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import net.sf.cglib.proxy.Callback;
import net.sf.cglib.proxy.Enhancer;

import com.comphenix.protocol.error.ErrorReporter;
import com.comphenix.protocol.events.PacketContainer;
import com.comphenix.protocol.events.PacketEvent;
import com.comphenix.protocol.injector.player.PlayerInjectionHandler;
Expand All @@ -51,6 +52,9 @@ class PacketInjector {
// The packet filter manager
private ListenerInvoker manager;

// Error reporter
private ErrorReporter reporter;

// Allows us to determine the sender
private PlayerInjectionHandler playerInjection;

Expand All @@ -61,11 +65,12 @@ class PacketInjector {
private ClassLoader classLoader;

public PacketInjector(ClassLoader classLoader, ListenerInvoker manager,
PlayerInjectionHandler playerInjection) throws IllegalAccessException {
PlayerInjectionHandler playerInjection, ErrorReporter reporter) throws IllegalAccessException {

this.classLoader = classLoader;
this.manager = manager;
this.playerInjection = playerInjection;
this.reporter = reporter;
this.readModifier = new ConcurrentHashMap<Integer, ReadPacketModifier>();
initialize();
}
Expand Down Expand Up @@ -133,7 +138,7 @@ public boolean addPacketHandler(int packetID) {
Class proxy = ex.createClass();

// Create the proxy handler
ReadPacketModifier modifier = new ReadPacketModifier(packetID, this);
ReadPacketModifier modifier = new ReadPacketModifier(packetID, this, reporter);
readModifier.put(packetID, modifier);

// Add a static reference
Expand Down
50 changes: 30 additions & 20 deletions ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.WeakHashMap;

import com.comphenix.protocol.Packets;
import com.comphenix.protocol.error.ErrorReporter;
import com.comphenix.protocol.events.PacketContainer;
import com.comphenix.protocol.events.PacketEvent;

Expand All @@ -44,12 +45,16 @@ class ReadPacketModifier implements MethodInterceptor {
private PacketInjector packetInjector;
private int packetID;

// Report errors
private ErrorReporter reporter;

// Whether or not a packet has been cancelled
private static Map<Object, Object> override = Collections.synchronizedMap(new WeakHashMap<Object, Object>());

public ReadPacketModifier(int packetID, PacketInjector packetInjector) {
public ReadPacketModifier(int packetID, PacketInjector packetInjector, ErrorReporter reporter) {
this.packetID = packetID;
this.packetInjector = packetInjector;
this.reporter = reporter;
}

/**
Expand Down Expand Up @@ -102,27 +107,32 @@ public Object intercept(Object thisObj, Method method, Object[] args, MethodProx
if (returnValue == null &&
Arrays.equals(method.getParameterTypes(), parameters)) {

// We need this in order to get the correct player
DataInputStream input = (DataInputStream) args[0];

// Let the people know
PacketContainer container = new PacketContainer(packetID, (Packet) thisObj);
PacketEvent event = packetInjector.packetRecieved(container, input);

// Handle override
if (event != null) {
Packet result = event.getPacket().getHandle();

if (event.isCancelled()) {
override.put(thisObj, CANCEL_MARKER);
} else if (!objectEquals(thisObj, result)) {
override.put(thisObj, result);
}
try {
// We need this in order to get the correct player
DataInputStream input = (DataInputStream) args[0];

// Let the people know
PacketContainer container = new PacketContainer(packetID, (Packet) thisObj);
PacketEvent event = packetInjector.packetRecieved(container, input);

// Update DataInputStream next time
if (!event.isCancelled() && packetID == Packets.Server.KEY_RESPONSE) {
packetInjector.scheduleDataInputRefresh(event.getPlayer());
// Handle override
if (event != null) {
Packet result = event.getPacket().getHandle();

if (event.isCancelled()) {
override.put(thisObj, CANCEL_MARKER);
} else if (!objectEquals(thisObj, result)) {
override.put(thisObj, result);
}

// Update DataInputStream next time
if (!event.isCancelled() && packetID == Packets.Server.KEY_RESPONSE) {
packetInjector.scheduleDataInputRefresh(event.getPlayer());
}
}
} catch (Throwable e) {
// Minecraft cannot handle this error
reporter.reportDetailed(this, "Cannot handle clienet packet.", e, args[0]);
}
}

Expand Down
7 changes: 6 additions & 1 deletion ...colLib/src/main/java/com/comphenix/protocol/injector/player/InjectedServerConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,12 @@ protected void onReplacing(Object inserting, Object replacement) {
// Is this a normal Minecraft object?
if (!(inserting instanceof Factory)) {
// If so, copy the content of the old element to the new
ObjectCloner.copyTo(inserting, replacement, inserting.getClass());
try {
ObjectCloner.copyTo(inserting, replacement, inserting.getClass());
} catch (Throwable e) {
reporter.reportDetailed(InjectedServerConnection.this, "Cannot copy old " + inserting +
" to new.", e, inserting, replacement);
}
}
}

Expand Down
48 changes: 26 additions & 22 deletions ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,30 +95,34 @@ public synchronized void cleanup(Object removing) {
PlayerInjector injected = injectedLogins.get(removing);

if (injected != null) {
PlayerInjector newInjector = null;
Player player = injected.getPlayer();

// Clean up list
injectedLogins.remove(removing);

// No need to clean up twice
if (injected.isClean())
return;

// Hack to clean up other references
newInjector = injectionHandler.getInjectorByNetworkHandler(injected.getNetworkManager());

// Update NetworkManager
if (newInjector == null) {
injectionHandler.uninjectPlayer(player);
} else {
injectionHandler.uninjectPlayer(player, false);
try {
PlayerInjector newInjector = null;
Player player = injected.getPlayer();

// Clean up list
injectedLogins.remove(removing);

if (injected instanceof NetworkObjectInjector)
newInjector.setNetworkManager(injected.getNetworkManager(), true);
// No need to clean up twice
if (injected.isClean())
return;

// Hack to clean up other references
newInjector = injectionHandler.getInjectorByNetworkHandler(injected.getNetworkManager());

// Update NetworkManager
if (newInjector == null) {
injectionHandler.uninjectPlayer(player);
} else {
injectionHandler.uninjectPlayer(player, false);

if (injected instanceof NetworkObjectInjector)
newInjector.setNetworkManager(injected.getNetworkManager(), true);
}

} catch (Throwable e) {
// Don't leak this to Minecraft
reporter.reportDetailed(this, "Cannot cleanup NetLoginHandler.", e, removing);
}

//logger.warning("Using alternative cleanup method.");
}
}

Expand Down
71 changes: 38 additions & 33 deletions ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -502,43 +502,48 @@ public boolean isClean() {
* @return The given packet, or the packet replaced by the listeners.
*/
public Packet handlePacketSending(Packet packet) {
// Get the packet ID too
Integer id = invoker.getPacketID(packet);
Player currentPlayer = player;

// Hack #1: Handle a single scheduled action
if (scheduledAction != null) {
scheduledAction.run();
scheduledAction = null;
}
// Hack #2
if (updateOnLogin) {
if (id == Packets.Server.LOGIN) {
try {
updatedPlayer = getEntityPlayer(getNetHandler()).getBukkitEntity();
} catch (IllegalAccessException e) {
reporter.reportDetailed(this, "Cannot update player in PlayerEvent.", e, packet);
try {
// Get the packet ID too
Integer id = invoker.getPacketID(packet);
Player currentPlayer = player;

// Hack #1: Handle a single scheduled action
if (scheduledAction != null) {
scheduledAction.run();
scheduledAction = null;
}
// Hack #2
if (updateOnLogin) {
if (id == Packets.Server.LOGIN) {
try {
updatedPlayer = getEntityPlayer(getNetHandler()).getBukkitEntity();
} catch (IllegalAccessException e) {
reporter.reportDetailed(this, "Cannot update player in PlayerEvent.", e, packet);
}
}

// This will only occur in the NetLoginHandler injection
if (updatedPlayer != null)
currentPlayer = updatedPlayer;
}

// This will only occur in the NetLoginHandler injection
if (updatedPlayer != null)
currentPlayer = updatedPlayer;
}

// Make sure we're listening
if (id != null && hasListener(id)) {
// A packet has been sent guys!
PacketContainer container = new PacketContainer(id, packet);
PacketEvent event = PacketEvent.fromServer(invoker, container, currentPlayer);
invoker.invokePacketSending(event);

// Cancelling is pretty simple. Just ignore the packet.
if (event.isCancelled())
return null;
// Make sure we're listening
if (id != null && hasListener(id)) {
// A packet has been sent guys!
PacketContainer container = new PacketContainer(id, packet);
PacketEvent event = PacketEvent.fromServer(invoker, container, currentPlayer);
invoker.invokePacketSending(event);

// Cancelling is pretty simple. Just ignore the packet.
if (event.isCancelled())
return null;

// Right, remember to replace the packet again
return event.getPacket().getHandle();
}

// Right, remember to replace the packet again
return event.getPacket().getHandle();
} catch (Throwable e) {
reporter.reportDetailed(this, "Cannot handle server packet.", e, packet);
}

return packet;
Expand Down

0 comments on commit 0dc2bfe

Please sign in to comment.