Bad fix of hasPermission() on null

[SOF3]

Issue description

Currently, when a plugin kicks a player in PlayerLoginEvent, etc., the server will continue to execute code that handles the client without checking whether the player is online. While there are no errors, it is WRONG for the server to continue doing stuff on a player that isn't online, assuming it is online.

Previously, this problem was mainly reflected by the internal call on Player::hasPermission that leads to a crash. Although an explicit null check is done on hasPermission, calling hasPermission on a closed player should indeed raise a warning (through throwing an exception) rather than returning a boolean false, as some plugins may hence cache and assume that the answer is false, while the actual value is undefined.

Vice versa. This also applies to myriad event calls related to players. Refer to comments below.

Possible resolution

1. Override Event::isCancelled in PlayerEvent (and probably EntityEvent too), and add a check to confirm that the player is online:

public function PlayerEvent::isCancelled() : bool{
  return parent::isCancelled() and $this->player->isOnline();
}
public function EntityEvent::isCancelled() : bool{
  return parent::isCancelled() and !($this->entity instanceof Player and !$this->entity->isOnline());
}

1. Override the new override methods like PlayerEvent::isCancelled for events that should not implement such check, such as PlayerQuitEvent:

public function PlayerQuitEvent::isCancelled() : bool{
  return Event::isCancelled();
}

Side effects

Slight backwards incompatibility. We cannot ask older plugins that declare events like PlayerQuitEvent to override isCancelled.

Resolution of side effects

I have been unable to come up with any resolution of side effects.

However, according to @shoghicp, the main author of the PocketMine event and plugin system, plugins SHOULD NOT extend PlayerEvent, etc. All events declared by plugins should extend PluginEvent. (It is believed that an exception is plugin events that share same handler lists as default events are not included by this statement, although this is not a good idea either)

For old plugins that extend PluginEvent, they are totally not covered by the proposed change in the Possible Resolution section above. The issues with them may or may not continue to exist, but they will only have the chance to get better, not worse, anyway.

Steps to reproduce the issue

Write a plugin that kicks a player in Player(Pre)?LoginEvent without cancelling the event.

OS and versions

• Genisys: e85b6f1
• PHP: Doesnt' matter
• Server OS: Doesn't matter
• Game version: Doesn't matter

Crashdump, backtrace, memory dumps, plugins list or other files

Plugin used to produce the issue.

<?php

/**
 * @name KickOnJoin
 * @author SOFe
 * @version 1.0.0
 * @api 1.0.0
 * @main KickOnJoin
 */

class KickOnJoin extends \pocketmine\plugin\PluginBase implements \pocketmine\event\Listener{
    public function onEnable(){
        $this->getServer()->getPluginManager()->registerEvents($this, $this);
    }
    public function onLogin(\pocketmine\event\player\PlayerPreLoginEvent $event){
        $event->getplayer()->close("Test");
    }
}

[dschwartz783]

You're not supposed to to kick from a login event handler. You're supposed to cancel the event. That should make it behave correctly. Don't worry, I've seen a lot of plugin devs get that wrong...

[dktapps]

^_^ it's still an issue however, and should be catered for.

[SOF3]

@dschwartz783 this also goes to other events, such as block placement, block interaction, entity damage, etc.

In fact, cancelled or not, PlayerInteractEvent still seems to be executing code related to players.

[SOF3]

@Muqsit the issues about other events is still not fully resolved.

[dktapps]

This is not going to be straightforward. I added an exception throw for getting permissions of an offline player and that in itself caused a server crash when broadcasting the player quit message. This is a very widespread issue.

[dktapps]

On the plus side, this problem will only affect cancellables. Which means that PlayerQuitEvent wouldn't need such an override anyway.

[inxomnyaa]

Totally confirmed but. PurePerms crashes the server when I join (noone else), but it shouldn't kick me though

[iPocket]

Throw all the blame on me for the fix 😂