Skip to content

Commit

Permalink
Adds some sanity checks in PacketEvent.setPacket().
Browse files Browse the repository at this point in the history 
Thanks libraryaddict. :)
  • Loading branch information
@aadnk
aadnk committed Mar 22, 2014
1 parent 7adb654 commit 45e236b
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions ProtocolLib/src/main/java/com/comphenix/protocol/events/PacketEvent.java
View file
Expand Up @@ -167,6 +167,10 @@ public PacketContainer getPacket() {
public void setPacket(PacketContainer packet) {
if (readOnly)
throw new IllegalStateException("The packet event is read-only.");
if (packet == null)
throw new IllegalArgumentException("Cannot set packet to NULL. Use setCancelled() instead.");
if (this.packet != null && this.packet.getType() != packet.getType())

This comment has been minimized.

Sign in to view

Copy link
@yawkat

yawkat Mar 29, 2014

Contributor

Why is replacing the packet with a different one not allowed?

This comment has been minimized.

Sign in to view

Copy link
@aadnk

aadnk Mar 29, 2014

Author Owner

You can see the reasoning behind the change here.

But I'm open to other suggestions, if this change is too breaking.

This comment has been minimized.

Sign in to view

Copy link
@yawkat

yawkat Mar 29, 2014

Contributor

Would the same not apply for simply changing a packet variable as well? For example, if one plugin changes the entity type from chicken to sheep and the other plugin then doesn't do what it's supposed to because it only checks for chicken packets?

I can easily bypass this with reflection because my plugin is the only one on the server but it still doesn't seem right to me. I have no better idea of addressing the problem libraryaddict detailed though.

This comment has been minimized.

Sign in to view

Copy link
@aadnk

aadnk Mar 29, 2014

Author Owner

But you can only filter a packet listener by packet type, not by a specific "packet variable" like entity type. So a plugin couldn't simply assume it would only get chicken packets on a general server (unless it relied on packet transmission order, in which case it is too brittle anyway). The fact that you can change the packet type, and thus receive a packet of a different type than what you specified in your filter, is counter-intuitive for both the plugin that change the type (since your packet listener will not be executed) and the other listening plugins.

The real problem, is that I introduced the concept of filtered packet listeners in the first place. It was necessary back in the days of 1.2.5, though, as intercepting packets, especially client-side, was expensive. It's too late to change now, though.

But I think I ought to change this into a warning. I don't want to cause plugin crashes over this, especially not for servers that only use one plugin.

This comment has been minimized.

Sign in to view

Copy link
@yawkat

yawkat Mar 29, 2014

Contributor

Yes, I can understand what your problem with it is then. The only other way I could think of would be calling the new type's handler loop when the type is changed and aborting the current one but that could cause infinite loops in some cases.

This comment has been minimized.

Sign in to view

Copy link
@aadnk

aadnk Mar 29, 2014

Author Owner

I've switched to a warning in in this commit. It will only be printed once per packet type -> packet type transition, and without any stack trace (which can be enabled by setting "detailed error" to TRUE in the configuration file).

This comment has been minimized.

Sign in to view

Copy link
@yawkat

yawkat Mar 29, 2014

Contributor

Thank you. It will not break existing plugins but still make it fairly easy to debug.
throw new IllegalArgumentException("Cannot change packet type from " + this.packet.getType() + " to " + packet.getType());
this.packet = packet;
}

Expand Down

0 comments on commit 45e236b

Please sign in to comment.