From 269231c2283375b7a1794a0d03c1e1b05b9dcf4a Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 15 Nov 2021 22:52:05 +0000 Subject: [PATCH] Ban foreach(arrayWithStringKeys as k => v) this is not as good as phpstan/phpstan-src#769 (e.g. array_key_first()/array_key_last() aren't covered by this, nor is array_rand()) but it does eliminate the most infuriating cases where this usually crops up. --- build/generate-known-translation-apis.php | 5 +- build/make-release.php | 3 +- phpstan.neon.dist | 1 + src/MemoryManager.php | 2 +- src/Server.php | 2 +- src/crash/CrashDump.php | 2 +- src/network/mcpe/convert/ItemTranslator.php | 3 +- src/network/query/QueryInfo.php | 2 +- src/permission/PermissibleInternal.php | 8 +- src/permission/PermissionParser.php | 3 +- src/plugin/PluginBase.php | 3 +- src/plugin/PluginLoadabilityChecker.php | 2 +- src/plugin/PluginManager.php | 10 +-- src/stats/SendUsageTask.php | 4 +- src/utils/Config.php | 6 +- src/utils/Filesystem.php | 3 +- src/utils/StringToTParser.php | 2 +- src/utils/Utils.php | 16 ++++ src/wizard/SetupWizard.php | 3 +- src/world/format/io/WorldProviderManager.php | 3 +- src/world/generator/GeneratorManager.php | 2 +- .../rules/UnsafeForeachArrayOfStringRule.php | 90 +++++++++++++++++++ tools/generate-permission-doc.php | 3 +- tools/simulate-chunk-selector.php | 5 +- 24 files changed, 150 insertions(+), 33 deletions(-) create mode 100644 tests/phpstan/rules/UnsafeForeachArrayOfStringRule.php diff --git a/build/generate-known-translation-apis.php b/build/generate-known-translation-apis.php index b2703b513..feee9b183 100644 --- a/build/generate-known-translation-apis.php +++ b/build/generate-known-translation-apis.php @@ -24,6 +24,7 @@ declare(strict_types=1); namespace pocketmine\build\generate_known_translation_apis; use pocketmine\lang\Translatable; +use pocketmine\utils\Utils; use Webmozart\PathUtil\Path; use function array_map; use function count; @@ -100,7 +101,7 @@ final class KnownTranslationKeys{ HEADER; ksort($languageDefinitions, SORT_STRING); - foreach($languageDefinitions as $k => $_){ + foreach(Utils::stringifyKeys($languageDefinitions) as $k => $_){ echo "\tpublic const "; echo constantify($k); echo " = \"" . $k . "\";\n"; @@ -135,7 +136,7 @@ HEADER; $parameterRegex = '/{%(.+?)}/'; $translationContainerClass = (new \ReflectionClass(Translatable::class))->getShortName(); - foreach($languageDefinitions as $key => $value){ + foreach(Utils::stringifyKeys($languageDefinitions) as $key => $value){ $parameters = []; if(preg_match_all($parameterRegex, $value, $matches) > 0){ foreach($matches[1] as $parameterName){ diff --git a/build/make-release.php b/build/make-release.php index 90961c392..e3a3dad81 100644 --- a/build/make-release.php +++ b/build/make-release.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace pocketmine\build\make_release; +use pocketmine\utils\Utils; use pocketmine\utils\VersionString; use pocketmine\VersionInfo; use function array_keys; @@ -76,7 +77,7 @@ const ACCEPTED_OPTS = [ function main() : void{ $filteredOpts = []; - foreach(getopt("", ["current:", "next:", "channel:", "help"]) as $optName => $optValue){ + foreach(Utils::stringifyKeys(getopt("", ["current:", "next:", "channel:", "help"])) as $optName => $optValue){ if($optName === "help"){ fwrite(STDOUT, "Options:\n"); diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 9da6fe352..6fec9cfa9 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -13,6 +13,7 @@ includes: rules: - pocketmine\phpstan\rules\DisallowEnumComparisonRule + - pocketmine\phpstan\rules\UnsafeForeachArrayOfStringRule # - pocketmine\phpstan\rules\ThreadedSupportedTypesRule parameters: diff --git a/src/MemoryManager.php b/src/MemoryManager.php index f45647969..9c2642b26 100644 --- a/src/MemoryManager.php +++ b/src/MemoryManager.php @@ -368,7 +368,7 @@ class MemoryManager{ '_SESSION' => true ]; - foreach($GLOBALS as $varName => $value){ + foreach(Utils::stringifyKeys($GLOBALS) as $varName => $value){ if(isset($ignoredGlobals[$varName])){ continue; } diff --git a/src/Server.php b/src/Server.php index b384c2957..628c9a311 100644 --- a/src/Server.php +++ b/src/Server.php @@ -1201,7 +1201,7 @@ class Server{ * Unsubscribes from all broadcast channels. */ public function unsubscribeFromAllBroadcastChannels(CommandSender $subscriber) : void{ - foreach($this->broadcastSubscribers as $channelId => $recipients){ + foreach(Utils::stringifyKeys($this->broadcastSubscribers) as $channelId => $recipients){ $this->unsubscribeFromBroadcastChannel($channelId, $subscriber); } } diff --git a/src/crash/CrashDump.php b/src/crash/CrashDump.php index 1d595402e..2719e317c 100644 --- a/src/crash/CrashDump.php +++ b/src/crash/CrashDump.php @@ -350,7 +350,7 @@ class CrashDump{ $this->addLine("Zend version: " . zend_version()); $this->addLine("OS: " . PHP_OS . ", " . Utils::getOS()); $this->addLine("Composer libraries: "); - foreach($composerLibraries as $library => $libraryVersion){ + foreach(Utils::stringifyKeys($composerLibraries) as $library => $libraryVersion){ $this->addLine("- $library $libraryVersion"); } } diff --git a/src/network/mcpe/convert/ItemTranslator.php b/src/network/mcpe/convert/ItemTranslator.php index bffa6cb18..dd1dc9cd2 100644 --- a/src/network/mcpe/convert/ItemTranslator.php +++ b/src/network/mcpe/convert/ItemTranslator.php @@ -27,6 +27,7 @@ use pocketmine\data\bedrock\LegacyItemIdToStringIdMap; use pocketmine\network\mcpe\protocol\serializer\ItemTypeDictionary; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\SingletonTrait; +use pocketmine\utils\Utils; use Webmozart\PathUtil\Path; use function array_key_exists; use function file_get_contents; @@ -92,7 +93,7 @@ final class ItemTranslator{ } $simpleMappings[$newId] = $intId; } - foreach($legacyStringToIntMap->getStringToLegacyMap() as $stringId => $intId){ + foreach(Utils::stringifyKeys($legacyStringToIntMap->getStringToLegacyMap()) as $stringId => $intId){ if(isset($simpleMappings[$stringId])){ throw new \UnexpectedValueException("Old ID $stringId collides with new ID"); } diff --git a/src/network/query/QueryInfo.php b/src/network/query/QueryInfo.php index b6cb199ca..559f9a653 100644 --- a/src/network/query/QueryInfo.php +++ b/src/network/query/QueryInfo.php @@ -233,7 +233,7 @@ final class QueryInfo{ $query .= $key . "\x00" . $value . "\x00"; } - foreach($this->extraData as $key => $value){ + foreach(Utils::stringifyKeys($this->extraData) as $key => $value){ $query .= $key . "\x00" . $value . "\x00"; } diff --git a/src/permission/PermissibleInternal.php b/src/permission/PermissibleInternal.php index 0d14d7943..14cb0ba1a 100644 --- a/src/permission/PermissibleInternal.php +++ b/src/permission/PermissibleInternal.php @@ -27,6 +27,7 @@ use pocketmine\plugin\Plugin; use pocketmine\plugin\PluginException; use pocketmine\timings\Timings; use pocketmine\utils\ObjectSet; +use pocketmine\utils\Utils; use function count; use function spl_object_id; @@ -143,7 +144,7 @@ class PermissibleInternal implements Permissible{ $oldPermissions = $this->permissions; $this->permissions = []; - foreach($this->rootPermissions as $name => $isGranted){ + foreach(Utils::stringifyKeys($this->rootPermissions) as $name => $isGranted){ $perm = $permManager->getPermission($name); if($perm === null){ throw new \LogicException("Unregistered root permission $name"); @@ -187,11 +188,12 @@ class PermissibleInternal implements Permissible{ } /** - * @param bool[] $children + * @param bool[] $children + * @phpstan-param array $children */ private function calculateChildPermissions(array $children, bool $invert, ?PermissionAttachment $attachment, ?PermissionAttachmentInfo $parent) : void{ $permManager = PermissionManager::getInstance(); - foreach($children as $name => $v){ + foreach(Utils::stringifyKeys($children) as $name => $v){ $perm = $permManager->getPermission($name); $value = ($v xor $invert); $this->permissions[$name] = new PermissionAttachmentInfo($name, $attachment, $value, $parent); diff --git a/src/permission/PermissionParser.php b/src/permission/PermissionParser.php index c7733a49c..8aab762cf 100644 --- a/src/permission/PermissionParser.php +++ b/src/permission/PermissionParser.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace pocketmine\permission; +use pocketmine\utils\Utils; use function is_bool; use function strtolower; @@ -83,7 +84,7 @@ class PermissionParser{ */ public static function loadPermissions(array $data, string $default = self::DEFAULT_FALSE) : array{ $result = []; - foreach($data as $name => $entry){ + foreach(Utils::stringifyKeys($data) as $name => $entry){ $desc = null; if(isset($entry["default"])){ $default = PermissionParser::defaultFromString($entry["default"]); diff --git a/src/plugin/PluginBase.php b/src/plugin/PluginBase.php index 7af6c3492..32b30794e 100644 --- a/src/plugin/PluginBase.php +++ b/src/plugin/PluginBase.php @@ -32,6 +32,7 @@ use pocketmine\scheduler\TaskScheduler; use pocketmine\Server; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\Config; +use pocketmine\utils\Utils; use Webmozart\PathUtil\Path; use function count; use function dirname; @@ -162,7 +163,7 @@ abstract class PluginBase implements Plugin, CommandExecutor{ private function registerYamlCommands() : void{ $pluginCmds = []; - foreach($this->getDescription()->getCommands() as $key => $data){ + foreach(Utils::stringifyKeys($this->getDescription()->getCommands()) as $key => $data){ if(strpos($key, ":") !== false){ $this->logger->error($this->server->getLanguage()->translate(KnownTranslationFactory::pocketmine_plugin_commandError($key, $this->getDescription()->getFullName(), ":"))); continue; diff --git a/src/plugin/PluginLoadabilityChecker.php b/src/plugin/PluginLoadabilityChecker.php index 9d3ca0387..a293f33c0 100644 --- a/src/plugin/PluginLoadabilityChecker.php +++ b/src/plugin/PluginLoadabilityChecker.php @@ -76,7 +76,7 @@ final class PluginLoadabilityChecker{ } } - foreach($description->getRequiredExtensions() as $extensionName => $versionConstrs){ + foreach(Utils::stringifyKeys($description->getRequiredExtensions()) as $extensionName => $versionConstrs){ $gotVersion = phpversion($extensionName); if($gotVersion === false){ return KnownTranslationFactory::pocketmine_plugin_extensionNotLoaded($extensionName); diff --git a/src/plugin/PluginManager.php b/src/plugin/PluginManager.php index 46986946c..de8413097 100644 --- a/src/plugin/PluginManager.php +++ b/src/plugin/PluginManager.php @@ -181,7 +181,7 @@ class PluginManager{ } $opRoot = $permManager->getPermission(DefaultPermissions::ROOT_OPERATOR); $everyoneRoot = $permManager->getPermission(DefaultPermissions::ROOT_USER); - foreach($description->getPermissions() as $default => $perms){ + foreach(Utils::stringifyKeys($description->getPermissions()) as $default => $perms){ foreach($perms as $perm){ $permManager->addPermission($perm); switch($default){ @@ -345,7 +345,7 @@ class PluginManager{ while(count($triage->plugins) > 0){ $loadedThisLoop = 0; - foreach($triage->plugins as $name => $entry){ + foreach(Utils::stringifyKeys($triage->plugins) as $name => $entry){ $this->checkDepsForTriage($name, "hard", $triage->dependencies, $loadedPlugins, $triage); $this->checkDepsForTriage($name, "soft", $triage->softDependencies, $loadedPlugins, $triage); @@ -377,7 +377,7 @@ class PluginManager{ //No plugins loaded :( //check for skippable soft dependencies first, in case the dependents could resolve hard dependencies - foreach($triage->plugins as $name => $file){ + foreach(Utils::stringifyKeys($triage->plugins) as $name => $file){ if(isset($triage->softDependencies[$name]) && !isset($triage->dependencies[$name])){ foreach($triage->softDependencies[$name] as $k => $dependency){ if($this->getPlugin($dependency) === null && !array_key_exists($dependency, $triage->plugins)){ @@ -392,7 +392,7 @@ class PluginManager{ } } - foreach($triage->plugins as $name => $file){ + foreach(Utils::stringifyKeys($triage->plugins) as $name => $file){ if(isset($triage->dependencies[$name])){ $unknownDependencies = []; @@ -415,7 +415,7 @@ class PluginManager{ } } - foreach($triage->plugins as $name => $file){ + foreach(Utils::stringifyKeys($triage->plugins) as $name => $file){ $this->server->getLogger()->critical($this->server->getLanguage()->translate(KnownTranslationFactory::pocketmine_plugin_loadError($name, KnownTranslationFactory::pocketmine_plugin_circularDependency()))); } break; diff --git a/src/stats/SendUsageTask.php b/src/stats/SendUsageTask.php index d108ca94a..9440ae779 100644 --- a/src/stats/SendUsageTask.php +++ b/src/stats/SendUsageTask.php @@ -123,9 +123,7 @@ class SendUsageTask extends AsyncTask{ ]; //This anonymizes the user ids so they cannot be reversed to the original - foreach($playerList as $k => $v){ - $playerList[$k] = md5($v); - } + $playerList = array_map('md5', $playerList); $players = array_map(function(Player $p) : string{ return md5($p->getUniqueId()->getBytes()); }, $server->getOnlinePlayers()); diff --git a/src/utils/Config.php b/src/utils/Config.php index 562d614f8..1a5125496 100644 --- a/src/utils/Config.php +++ b/src/utils/Config.php @@ -425,7 +425,7 @@ class Config{ public function set($k, $v = true) : void{ $this->config[$k] = $v; $this->changed = true; - foreach($this->nestedCache as $nestedKey => $nvalue){ + foreach(Utils::stringifyKeys($this->nestedCache) as $nestedKey => $nvalue){ if(substr($nestedKey, 0, strlen($k) + 1) === ($k . ".")){ unset($this->nestedCache[$nestedKey]); } @@ -487,7 +487,7 @@ class Config{ */ private function fillDefaults(array $default, &$data) : int{ $changed = 0; - foreach($default as $k => $v){ + foreach(Utils::stringifyKeys($default) as $k => $v){ if(is_array($v)){ if(!isset($data[$k]) or !is_array($data[$k])){ $data[$k] = []; @@ -536,7 +536,7 @@ class Config{ */ public static function writeProperties(array $config) : string{ $content = "#Properties Config file\r\n#" . date("D M j H:i:s T Y") . "\r\n"; - foreach($config as $k => $v){ + foreach(Utils::stringifyKeys($config) as $k => $v){ if(is_bool($v)){ $v = $v ? "on" : "off"; } diff --git a/src/utils/Filesystem.php b/src/utils/Filesystem.php index 9a89976e6..2d107af6e 100644 --- a/src/utils/Filesystem.php +++ b/src/utils/Filesystem.php @@ -163,7 +163,8 @@ final class Filesystem{ $result = str_replace([DIRECTORY_SEPARATOR, ".php", "phar://"], ["/", "", ""], $path); //remove relative paths - foreach(self::$cleanedPaths as $cleanPath => $replacement){ + //this should probably never have integer keys, but it's safer than making PHPStan ignore it + foreach(Utils::stringifyKeys(self::$cleanedPaths) as $cleanPath => $replacement){ $cleanPath = rtrim(str_replace([DIRECTORY_SEPARATOR, "phar://"], ["/", ""], $cleanPath), "/"); if(strpos($result, $cleanPath) === 0){ $result = ltrim(str_replace($cleanPath, $replacement, $result), "/"); diff --git a/src/utils/StringToTParser.php b/src/utils/StringToTParser.php index 598b3768d..a15204ba4 100644 --- a/src/utils/StringToTParser.php +++ b/src/utils/StringToTParser.php @@ -75,7 +75,7 @@ abstract class StringToTParser{ return strtolower(str_replace([" ", "minecraft:"], ["_", ""], trim($input))); } - /** @return string[] */ + /** @return string[]|int[] */ public function getKnownAliases() : array{ return array_keys($this->callbackMap); } diff --git a/src/utils/Utils.php b/src/utils/Utils.php index b0e81ca28..6f9500125 100644 --- a/src/utils/Utils.php +++ b/src/utils/Utils.php @@ -571,6 +571,22 @@ final class Utils{ } } + /** + * Generator which forces array keys to string during iteration. + * This is necessary because PHP has an anti-feature where it casts numeric string keys to integers, leading to + * various crashes. + * + * @phpstan-template TKeyType of string + * @phpstan-template TValueType + * @phpstan-param array $array + * @phpstan-return \Generator + */ + public static function stringifyKeys(array $array) : \Generator{ + foreach($array as $key => $value){ // @phpstan-ignore-line - this is where we fix the stupid bullshit with array keys :) + yield (string) $key => $value; + } + } + public static function checkUTF8(string $string) : void{ if(!mb_check_encoding($string, 'UTF-8')){ throw new \InvalidArgumentException("Text must be valid UTF-8"); diff --git a/src/wizard/SetupWizard.php b/src/wizard/SetupWizard.php index bb1b8de26..6dfb3b690 100644 --- a/src/wizard/SetupWizard.php +++ b/src/wizard/SetupWizard.php @@ -35,6 +35,7 @@ use pocketmine\player\GameMode; use pocketmine\utils\Config; use pocketmine\utils\Internet; use pocketmine\utils\InternetException; +use pocketmine\utils\Utils; use pocketmine\VersionInfo; use Webmozart\PathUtil\Path; use function fgets; @@ -69,7 +70,7 @@ class SetupWizard{ } $this->message("Please select a language"); - foreach($langs as $short => $native){ + foreach(Utils::stringifyKeys($langs) as $short => $native){ $this->writeLine(" $native => $short"); } diff --git a/src/world/format/io/WorldProviderManager.php b/src/world/format/io/WorldProviderManager.php index 001f92913..64dad0816 100644 --- a/src/world/format/io/WorldProviderManager.php +++ b/src/world/format/io/WorldProviderManager.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace pocketmine\world\format\io; +use pocketmine\utils\Utils; use pocketmine\world\format\io\leveldb\LevelDB; use pocketmine\world\format\io\region\Anvil; use pocketmine\world\format\io\region\McRegion; @@ -77,7 +78,7 @@ final class WorldProviderManager{ */ public function getMatchingProviders(string $path) : array{ $result = []; - foreach($this->providers as $alias => $providerEntry){ + foreach(Utils::stringifyKeys($this->providers) as $alias => $providerEntry){ if($providerEntry->isValid($path)){ $result[$alias] = $providerEntry; } diff --git a/src/world/generator/GeneratorManager.php b/src/world/generator/GeneratorManager.php index 11fd02f3b..0e29cc68b 100644 --- a/src/world/generator/GeneratorManager.php +++ b/src/world/generator/GeneratorManager.php @@ -105,7 +105,7 @@ final class GeneratorManager{ */ public function getGeneratorName(string $class) : string{ Utils::testValidInstance($class, Generator::class); - foreach($this->list as $name => $c){ + foreach(Utils::stringifyKeys($this->list) as $name => $c){ if($c->getGeneratorClass() === $class){ return $name; } diff --git a/tests/phpstan/rules/UnsafeForeachArrayOfStringRule.php b/tests/phpstan/rules/UnsafeForeachArrayOfStringRule.php new file mode 100644 index 000000000..639a5b469 --- /dev/null +++ b/tests/phpstan/rules/UnsafeForeachArrayOfStringRule.php @@ -0,0 +1,90 @@ + + */ +final class UnsafeForeachArrayOfStringRule implements Rule{ + + public function getNodeType() : string{ + return Foreach_::class; + } + + public function processNode(Node $node, Scope $scope) : array{ + /** @var Foreach_ $node */ + if($node->keyVar === null){ + return []; + } + $iterableType = $scope->getType($node->expr); + + if($iterableType->isArray()->no()){ + return []; + } + if($iterableType->isIterableAtLeastOnce()->no()){ + return []; + } + + $hasCastableKeyTypes = false; + $expectsIntKeyTypes = false; + TypeTraverser::map($iterableType->getIterableKeyType(), function(Type $type, callable $traverse) use (&$hasCastableKeyTypes, &$expectsIntKeyTypes) : Type{ + if($type instanceof IntegerType){ + $expectsIntKeyTypes = true; + return $type; + } + if(!$type instanceof StringType){ + return $traverse($type); + } + if($type->isNumericString()->no() || $type instanceof ClassStringType){ + //class-string cannot be numeric, even if PHPStan thinks they can be + return $type; + } + $hasCastableKeyTypes = true; + return $type; + }); + if($hasCastableKeyTypes && !$expectsIntKeyTypes){ + return [ + RuleErrorBuilder::message(sprintf( + "Unsafe foreach on array with key type %s (they might be casted to int).", + $iterableType->getIterableKeyType()->describe(VerbosityLevel::value()) + ))->tip("Use Utils::foreachWithStringKeys() for a safe Generator-based iterator.")->build() + ]; + } + return []; + } + +} \ No newline at end of file diff --git a/tools/generate-permission-doc.php b/tools/generate-permission-doc.php index 57fd04626..944d2aae3 100644 --- a/tools/generate-permission-doc.php +++ b/tools/generate-permission-doc.php @@ -26,6 +26,7 @@ namespace pocketmine\generate_permission_doc; use pocketmine\permission\DefaultPermissions; use pocketmine\permission\PermissionManager; use pocketmine\utils\AssumptionFailedError; +use pocketmine\utils\Utils; use pocketmine\VersionInfo; use Webmozart\PathUtil\Path; use function count; @@ -90,7 +91,7 @@ foreach($permissions as $permission){ fwrite($doc, "|:-----|:----:|\n"); $children = $permission->getChildren(); ksort($children, SORT_STRING); - foreach($children as $childName => $isGranted){ + foreach(Utils::stringifyKeys($children) as $childName => $isGranted){ fwrite($doc, "| `$childName` | " . ($isGranted ? "Granted" : "Denied") . " |\n"); } fwrite($doc, "\n"); diff --git a/tools/simulate-chunk-selector.php b/tools/simulate-chunk-selector.php index b4af186e2..b30355f7c 100644 --- a/tools/simulate-chunk-selector.php +++ b/tools/simulate-chunk-selector.php @@ -6,6 +6,7 @@ namespace pocketmine\tools\simulate_chunk_selector; use pocketmine\player\ChunkSelector; use pocketmine\utils\AssumptionFailedError; +use pocketmine\utils\Utils; use pocketmine\world\format\Chunk; use pocketmine\world\World; use Webmozart\PathUtil\Path; @@ -115,7 +116,7 @@ if(count(getopt("", ["help"])) !== 0){ exit(0); } -foreach(getopt("", ["radius:", "baseX:", "baseZ:", "scale:", "chunksPerStep:"]) as $name => $value){ +foreach(Utils::stringifyKeys(getopt("", ["radius:", "baseX:", "baseZ:", "scale:", "chunksPerStep:"])) as $name => $value){ if(!is_string($value) || (string) ((int) $value) !== $value){ fwrite(STDERR, "Value for --$name must be an integer\n"); exit(1); @@ -136,7 +137,7 @@ if($radius === null){ } $outputDirectory = null; -foreach(getopt("", ["output:"]) as $name => $value){ +foreach(Utils::stringifyKeys(getopt("", ["output:"])) as $name => $value){ assert($name === "output"); if(!is_string($value)){ fwrite(STDERR, "Value for --$name must be a string\n");