From 836cb67850973a0c21db64c415f43a863b89cdd0 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 6 Jan 2019 17:22:31 +0000 Subject: [PATCH 1/6] Server: don't abuse random_bytes() to generate integers --- src/pocketmine/Server.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index ed5f26b96..f5ec53406 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -98,7 +98,6 @@ use pocketmine\tile\Tile; use pocketmine\timings\Timings; use pocketmine\timings\TimingsHandler; use pocketmine\updater\AutoUpdater; -use pocketmine\utils\Binary; use pocketmine\utils\Config; use pocketmine\utils\Internet; use pocketmine\utils\MainLogger; @@ -148,6 +147,7 @@ use function pcntl_signal; use function pcntl_signal_dispatch; use function preg_replace; use function random_bytes; +use function random_int; use function realpath; use function register_shutdown_function; use function rename; @@ -167,6 +167,8 @@ use function time; use function touch; use function trim; use const DIRECTORY_SEPARATOR; +use const INT32_MAX; +use const INT32_MIN; use const PHP_EOL; use const PHP_INT_MAX; use const PTHREADS_INHERIT_NONE; @@ -1127,7 +1129,7 @@ class Server{ return false; } - $seed = $seed ?? Binary::readInt(random_bytes(4)); + $seed = $seed ?? random_int(INT32_MIN, INT32_MAX); if(!isset($options["preset"])){ $options["preset"] = $this->getConfigString("generator-settings", ""); From 2cdf97b7b50bb458f1bec252f59dea8c57155cf7 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 16 Dec 2018 19:34:40 +0000 Subject: [PATCH 2/6] CrashDump: Scan full stack trace to determine plugin involvement --- src/pocketmine/CrashDump.php | 49 ++++++++++++++++++++++------------ src/pocketmine/Server.php | 2 +- src/pocketmine/utils/Utils.php | 16 ++++++++++- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/pocketmine/CrashDump.php b/src/pocketmine/CrashDump.php index 8cefb4912..f83b9b650 100644 --- a/src/pocketmine/CrashDump.php +++ b/src/pocketmine/CrashDump.php @@ -207,7 +207,7 @@ class CrashDump{ $error = $lastExceptionError; }else{ $error = (array) error_get_last(); - $error["trace"] = Utils::printableCurrentTrace(3); //Skipping CrashDump->baseCrash, CrashDump->construct, Server->crashDump + $error["trace"] = Utils::currentTrace(3); //Skipping CrashDump->baseCrash, CrashDump->construct, Server->crashDump $errorConversion = [ E_ERROR => "E_ERROR", E_WARNING => "E_WARNING", @@ -245,24 +245,16 @@ class CrashDump{ $this->addLine("Line: " . $error["line"]); $this->addLine("Type: " . $error["type"]); - if(strpos($error["file"], "src/pocketmine/") === false and strpos($error["file"], "vendor/pocketmine/") === false and file_exists($error["fullFile"])){ - $this->addLine(); - $this->addLine("THIS CRASH WAS CAUSED BY A PLUGIN"); - $this->data["plugin"] = true; - - $reflection = new \ReflectionClass(PluginBase::class); - $file = $reflection->getProperty("file"); - $file->setAccessible(true); - foreach($this->server->getPluginManager()->getPlugins() as $plugin){ - $filePath = Utils::cleanPath($file->getValue($plugin)); - if(strpos($error["file"], $filePath) === 0){ - $this->data["plugin"] = $plugin->getName(); - $this->addLine("BAD PLUGIN: " . $plugin->getDescription()->getFullName()); + $this->data["plugin"] = false; + if(!$this->determinePluginFromFile($error["fullFile"])){ //fatal errors won't leave any stack trace + foreach($error["trace"] as $frame){ + if(!isset($frame["file"])){ + continue; //PHP core + } + if($this->determinePluginFromFile($frame["file"])){ break; } } - }else{ - $this->data["plugin"] = false; } $this->addLine(); @@ -279,12 +271,35 @@ class CrashDump{ $this->addLine(); $this->addLine("Backtrace:"); - foreach(($this->data["trace"] = $error["trace"]) as $line){ + foreach(($this->data["trace"] = Utils::printableTrace($error["trace"])) as $line){ $this->addLine($line); } $this->addLine(); } + private function determinePluginFromFile(string $filePath) : bool{ + $frameCleanPath = Utils::cleanPath($filePath); //this will be empty in phar stub + if($frameCleanPath !== "" and strpos($frameCleanPath, "src/pocketmine/") === false and strpos($frameCleanPath, "vendor/pocketmine/") === false and file_exists($filePath)){ + $this->addLine(); + $this->addLine("THIS CRASH WAS CAUSED BY A PLUGIN"); + $this->data["plugin"] = true; + + $reflection = new \ReflectionClass(PluginBase::class); + $file = $reflection->getProperty("file"); + $file->setAccessible(true); + foreach($this->server->getPluginManager()->getPlugins() as $plugin){ + $filePath = Utils::cleanPath($file->getValue($plugin)); + if(strpos($frameCleanPath, $filePath) === 0){ + $this->data["plugin"] = $plugin->getName(); + $this->addLine("BAD PLUGIN: " . $plugin->getDescription()->getFullName()); + break; + } + } + return true; + } + return false; + } + private function generalData(){ $version = new VersionString(\pocketmine\BASE_VERSION, \pocketmine\IS_DEVELOPMENT_BUILD, \pocketmine\BUILD_NUMBER); $this->data["general"] = []; diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index f5ec53406..926285250 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -2241,7 +2241,7 @@ class Server{ "fullFile" => $e->getFile(), "file" => $errfile, "line" => $errline, - "trace" => Utils::printableTrace($trace) + "trace" => $trace ]; global $lastExceptionError, $lastError; diff --git a/src/pocketmine/utils/Utils.php b/src/pocketmine/utils/Utils.php index 93e3322b1..5e69755b5 100644 --- a/src/pocketmine/utils/Utils.php +++ b/src/pocketmine/utils/Utils.php @@ -642,7 +642,21 @@ class Utils{ } public static function cleanPath($path){ - return str_replace(["\\", ".php", "phar://", str_replace(["\\", "phar://"], ["/", ""], \pocketmine\PATH), str_replace(["\\", "phar://"], ["/", ""], \pocketmine\PLUGIN_PATH)], ["/", "", "", "", ""], $path); + $result = str_replace(["\\", ".php", "phar://"], ["/", "", ""], $path); + + //remove relative paths + //TODO: make these paths dynamic so they can be unit-tested against + static $cleanPaths = [ + \pocketmine\PLUGIN_PATH => "plugins", //this has to come BEFORE \pocketmine\PATH because it's inside that by default on src installations + \pocketmine\PATH => "" + ]; + foreach($cleanPaths as $cleanPath => $replacement){ + $cleanPath = rtrim(str_replace(["\\", "phar://"], ["/", ""], $cleanPath), "/"); + if(strpos($result, $cleanPath) === 0){ + $result = ltrim(str_replace($cleanPath, $replacement, $result), "/"); + } + } + return $result; } /** From 22a6b817d7e12c1b7167a0b3b9d89152b13865a3 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 27 Dec 2018 17:18:46 +0000 Subject: [PATCH 3/6] Distinguish between direct and indirect plugin crash involvement If a plugin was involved in a crash, we can't safely blame it for the crash, since it might have innocently triggered a core bug. Furthermore, it's difficult to accurately detect plugin causing things like invalid argument crashes. --- src/pocketmine/CrashDump.php | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/pocketmine/CrashDump.php b/src/pocketmine/CrashDump.php index f83b9b650..353a423fe 100644 --- a/src/pocketmine/CrashDump.php +++ b/src/pocketmine/CrashDump.php @@ -87,7 +87,11 @@ class CrashDump{ * having their content changed, version format changing, etc. * It is not necessary to increase this when adding new fields. */ - private const FORMAT_VERSION = 1; + private const FORMAT_VERSION = 2; + + private const PLUGIN_INVOLVEMENT_NONE = "none"; + private const PLUGIN_INVOLVEMENT_DIRECT = "direct"; + private const PLUGIN_INVOLVEMENT_INDIRECT = "indirect"; /** @var Server */ private $server; @@ -245,13 +249,13 @@ class CrashDump{ $this->addLine("Line: " . $error["line"]); $this->addLine("Type: " . $error["type"]); - $this->data["plugin"] = false; - if(!$this->determinePluginFromFile($error["fullFile"])){ //fatal errors won't leave any stack trace + $this->data["plugin_involvement"] = self::PLUGIN_INVOLVEMENT_NONE; + if(!$this->determinePluginFromFile($error["fullFile"], true)){ //fatal errors won't leave any stack trace foreach($error["trace"] as $frame){ if(!isset($frame["file"])){ continue; //PHP core } - if($this->determinePluginFromFile($frame["file"])){ + if($this->determinePluginFromFile($frame["file"], false)){ break; } } @@ -277,12 +281,17 @@ class CrashDump{ $this->addLine(); } - private function determinePluginFromFile(string $filePath) : bool{ + private function determinePluginFromFile(string $filePath, bool $crashFrame) : bool{ $frameCleanPath = Utils::cleanPath($filePath); //this will be empty in phar stub if($frameCleanPath !== "" and strpos($frameCleanPath, "src/pocketmine/") === false and strpos($frameCleanPath, "vendor/pocketmine/") === false and file_exists($filePath)){ $this->addLine(); - $this->addLine("THIS CRASH WAS CAUSED BY A PLUGIN"); - $this->data["plugin"] = true; + if($crashFrame){ + $this->addLine("THIS CRASH WAS CAUSED BY A PLUGIN"); + $this->data["plugin_involvement"] = self::PLUGIN_INVOLVEMENT_DIRECT; + }else{ + $this->addLine("A PLUGIN WAS INVOLVED IN THIS CRASH"); + $this->data["plugin_involvement"] = self::PLUGIN_INVOLVEMENT_INDIRECT; + } $reflection = new \ReflectionClass(PluginBase::class); $file = $reflection->getProperty("file"); From a6e5b6e158ff82da1a5ee1f12376c37d4f25114c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 6 Jan 2019 19:52:20 +0000 Subject: [PATCH 4/6] Log a debug message when not sending crash due to folder plugin --- src/pocketmine/Server.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pocketmine/Server.php b/src/pocketmine/Server.php index 926285250..58665af1a 100644 --- a/src/pocketmine/Server.php +++ b/src/pocketmine/Server.php @@ -2280,7 +2280,7 @@ class Server{ if(is_string($plugin)){ $p = $this->pluginManager->getPlugin($plugin); if($p instanceof Plugin and !($p->getPluginLoader() instanceof PharPluginLoader)){ - $report = false; + $this->logger->debug("Not sending crashdump due to caused by non-phar plugin"); } } From c9e598cdb9cdf8901f7cad4794f58f9a8edca84c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 6 Jan 2019 19:55:21 +0000 Subject: [PATCH 5/6] Release 3.5.5 --- src/pocketmine/PocketMine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pocketmine/PocketMine.php b/src/pocketmine/PocketMine.php index eb9de5c90..af60518f2 100644 --- a/src/pocketmine/PocketMine.php +++ b/src/pocketmine/PocketMine.php @@ -38,7 +38,7 @@ namespace pocketmine { const NAME = "PocketMine-MP"; const BASE_VERSION = "3.5.5"; - const IS_DEVELOPMENT_BUILD = true; + const IS_DEVELOPMENT_BUILD = false; const BUILD_NUMBER = 0; const MIN_PHP_VERSION = "7.2.0"; From 6a9cad8fb7be34ada10bc52823bc5c9793b18b8c Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 6 Jan 2019 20:18:32 +0000 Subject: [PATCH 6/6] 3.5.6 is next --- src/pocketmine/PocketMine.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pocketmine/PocketMine.php b/src/pocketmine/PocketMine.php index af60518f2..62f6fdb84 100644 --- a/src/pocketmine/PocketMine.php +++ b/src/pocketmine/PocketMine.php @@ -37,8 +37,8 @@ namespace pocketmine { use pocketmine\wizard\SetupWizard; const NAME = "PocketMine-MP"; - const BASE_VERSION = "3.5.5"; - const IS_DEVELOPMENT_BUILD = false; + const BASE_VERSION = "3.5.6"; + const IS_DEVELOPMENT_BUILD = true; const BUILD_NUMBER = 0; const MIN_PHP_VERSION = "7.2.0";