Merge branch '3.6'

src/pocketmine/level/format/io/region/RegionLoader.php

@@ -119,9 +119,6 @@ class RegionLoader{
 	 */
 	public function readChunk(int $x, int $z) : ?string{
 		$index = self::getChunkOffset($x, $z);
-		if($index < 0 or $index >= 4096){
-			throw new \InvalidArgumentException("Invalid chunk position in region, expected x/z in range 0-31, got x=$x, z=$z");
-		}
 
 		$this->lastUsed = time();
 
@@ -140,13 +137,13 @@ class RegionLoader{
 			if($length >= self::MAX_SECTOR_LENGTH){
 				$this->locationTable[$index][0] = ++$this->lastSector;
 				$this->locationTable[$index][1] = 1;
-				throw new CorruptedChunkException("Corrupted chunk header detected (sector count larger than max)");
+				throw new CorruptedChunkException("Corrupted chunk header detected (sector count $length larger than max " . self::MAX_SECTOR_LENGTH . ")");
 			}
 			return null;
 		}
 
 		if($length > ($this->locationTable[$index][1] << 12)){ //Invalid chunk, bigger than defined number of sectors
-			\GlobalLogger::get()->error("Corrupted bigger chunk detected (bigger than number of sectors given in header)");
+			\GlobalLogger::get()->error("Chunk x=$x,z=$z length mismatch (expected " . ($this->locationTable[$index][1] << 12) . " sectors, got $length sectors)");
 			$this->locationTable[$index][1] = $length >> 12;
 			$this->writeLocationIndex($index);
 		}
@@ -164,10 +161,25 @@ class RegionLoader{
 		return substr($chunkData, 1);
 	}
 
+	/**
+	 * @param int $x
+	 * @param int $z
+	 *
+	 * @return bool
+	 * @throws \InvalidArgumentException
+	 */
 	public function chunkExists(int $x, int $z) : bool{
 		return $this->isChunkGenerated(self::getChunkOffset($x, $z));
 	}
 
+	/**
+	 * @param int    $x
+	 * @param int    $z
+	 * @param string $chunkData
+	 *
+	 * @throws ChunkException
+	 * @throws \InvalidArgumentException
+	 */
 	public function writeChunk(int $x, int $z, string $chunkData) : void{
 		$this->lastUsed = time();
 
@@ -197,14 +209,40 @@ class RegionLoader{
 		}
 	}
 
+	/**
+	 * @param int $x
+	 * @param int $z
+	 *
+	 * @throws \InvalidArgumentException
+	 */
 	public function removeChunk(int $x, int $z) : void{
 		$index = self::getChunkOffset($x, $z);
 		$this->locationTable[$index][0] = 0;
 		$this->locationTable[$index][1] = 0;
 	}
 
+	/**
+	 * @param int $x
+	 * @param int $z
+	 *
+	 * @return int
+	 * @throws \InvalidArgumentException
+	 */
 	protected static function getChunkOffset(int $x, int $z) : int{
-		return $x + ($z << 5);
+		if($x < 0 or $x > 31 or $z < 0 or $z > 31){
+			throw new \InvalidArgumentException("Invalid chunk position in region, expected x/z in range 0-31, got x=$x, z=$z");
+		}
+		return $x | ($z << 5);
+	}
+
+	/**
+	 * @param int $offset
+	 * @param int &$x
+	 * @param int &$z
+	 */
+	protected static function getChunkCoords(int $offset, ?int &$x, ?int &$z) : void{
+		$x = $offset & 0x1f;
+		$z = ($offset >> 5) & 0x1f;
 	}
 
 	/**
@@ -235,18 +273,23 @@ class RegionLoader{
 		}
 
 		$data = unpack("N*", $headerRaw);
+		/** @var int[] $usedOffsets */
 		$usedOffsets = [];
 		for($i = 0; $i < 1024; ++$i){
 			$index = $data[$i + 1];
 			$offset = $index >> 8;
 			if($offset !== 0){
-				fseek($this->filePointer, $offset << 12);
+				self::getChunkCoords($i, $x, $z);
+				$fileOffset = $offset << 12;
+
+				fseek($this->filePointer, $fileOffset);
 				if(fgetc($this->filePointer) === false){ //Try and read from the location
-					throw new CorruptedRegionException("Region file location offset points to invalid location");
+					throw new CorruptedRegionException("Region file location offset x=$x,z=$z points to invalid file location $fileOffset");
 				}elseif(isset($usedOffsets[$offset])){
-					throw new CorruptedRegionException("Found two chunk offsets pointing to the same location");
+					self::getChunkCoords($usedOffsets[$offset], $existingX, $existingZ);
+					throw new CorruptedRegionException("Found two chunk offsets (chunk1: x=$existingX,z=$existingZ, chunk2: x=$x,z=$z) pointing to the file location $fileOffset");
 				}else{
-					$usedOffsets[$offset] = true;
+					$usedOffsets[$offset] = $i;
 				}
 			}
 

tests/phpunit/level/format/io/region/RegionLoaderTest.php

@@ -25,28 +25,94 @@ namespace pocketmine\level\format\io\region;
 
 use PHPUnit\Framework\TestCase;
 use pocketmine\level\format\ChunkException;
+use function file_exists;
+use function random_bytes;
+use function str_repeat;
+use function sys_get_temp_dir;
+use function unlink;
 
 class RegionLoaderTest extends TestCase{
 
-	public function testChunkTooBig() : void{
+	/** @var string */
-		$r = new RegionLoader(sys_get_temp_dir() . '/chunk_too_big.testregion_' . bin2hex(random_bytes(4)));
+	private $regionPath;
-		$r->open();
+	/** @var RegionLoader */
+	private $region;
 
+	public function setUp(){
+		$this->regionPath = sys_get_temp_dir() . '/test.testregion';
+		if(file_exists($this->regionPath)){
+			unlink($this->regionPath);
+		}
+		$this->region = new RegionLoader($this->regionPath);
+		$this->region->open();
+	}
+
+	public function tearDown(){
+		$this->region->close();
+		if(file_exists($this->regionPath)){
+			unlink($this->regionPath);
+		}
+	}
+
+	public function testChunkTooBig() : void{
 		$this->expectException(ChunkException::class);
-		$r->writeChunk(0, 0, str_repeat("a", 1044476));
+		$this->region->writeChunk(0, 0, str_repeat("a", 1044476));
 	}
 
 	public function testChunkMaxSize() : void{
 		$data = str_repeat("a", 1044475);
-		$path = sys_get_temp_dir() . '/chunk_just_fits.testregion_' . bin2hex(random_bytes(4));
+		$this->region->writeChunk(0, 0, $data);
-		$r = new RegionLoader($path);
+		$this->region->close();
-		$r->open();
 
-		$r->writeChunk(0, 0, $data);
+		$r = new RegionLoader($this->regionPath);
-		$r->close();
-
-		$r = new RegionLoader($path);
 		$r->open();
 		self::assertSame($data, $r->readChunk(0, 0));
 	}
+
+	public function outOfBoundsCoordsProvider() : \Generator{
+		yield [-1, -1];
+		yield [32, 32];
+		yield [-1, 32];
+		yield [32, -1];
+	}
+
+	/**
+	 * @dataProvider outOfBoundsCoordsProvider
+	 * @param int $x
+	 * @param int $z
+	 *
+	 * @throws ChunkException
+	 * @throws \InvalidArgumentException
+	 */
+	public function testWriteChunkOutOfBounds(int $x, int $z) : void{
+		$this->expectException(\InvalidArgumentException::class);
+		$this->region->writeChunk($x, $z, str_repeat("\x00", 1000));
+	}
+
+	public function testReadWriteChunkInBounds() : void{
+		$dat = random_bytes(1000);
+		for($x = 0; $x < 32; ++$x){
+			for($z = 0; $z < 32; ++$z){
+				$this->region->writeChunk($x, $z, $dat);
+			}
+		}
+		for($x = 0; $x < 32; ++$x){
+			for($z = 0; $z < 32; ++$z){
+				self::assertSame($dat, $this->region->readChunk($x, $z));
+			}
+		}
+	}
+
+	/**
+	 * @dataProvider outOfBoundsCoordsProvider
+	 * @param int $x
+	 * @param int $z
+	 *
+	 * @throws \InvalidArgumentException
+	 * @throws \pocketmine\level\format\io\exception\CorruptedChunkException
+	 */
+	public function testReadChunkOutOfBounds(int $x, int $z) : void{
+		$this->expectException(\InvalidArgumentException::class);
+		$this->region->readChunk($x, $z);
+	}
 }