Skip to content

Added safety checks for region file validity#393

Merged
dktapps merged 1 commit into
masterfrom
region-safety
Apr 27, 2017
Merged

Added safety checks for region file validity#393
dktapps merged 1 commit into
masterfrom
region-safety

Conversation

@dktapps
Copy link
Copy Markdown
Member

@dktapps dktapps commented Mar 2, 2017

Recently it was brought to my attention, by someone having freeze issues on their server, that corrupted or invalid region files can really mess up your server.

In the process of testing their issue I discovered that the RegionLoader does not impose any checks whatsoever on region file validity. You can feed any random crap into there and PocketMine will attempt to use it.
My test:

  • create new anvil world
  • cd worlds/world/region
  • echo "lelelelelelelelel" > r.0.0.mca
  • start the server
  • join, teleport to 0 128 0
  • server freezes and writes out 60GB worth of crap to disk, machine freezes for about 15 minutes.

The reason for said freeze was due to an invalid location table. Region files begin with an 8192-byte header, the first 4096 bytes of which are chunk locations. The first 3 bytes of each entry is the file offset in 4KiB sectors, and the last byte is the length in sectors. Trying to read a garbage file like the one above produces some extremely large offsets for chunks that don't actually exist, so when the server writes to that offset, it also writes billions of zeros in between.

This is a very critical issue since the process cannot be killed while it is writing to disk. In the process of debugging this issue I had to force-kill my laptop half a dozen times.

This pull request adds tight restrictions on region file validity as per the specification here to prevent the issue described above occurring.

  • A maximum file size of 1GiB + 8192 bytes has been imposed for region files. The server will treat anything bigger than this as corrupted. A single region should never get this big, regardless.
  • Region files must now be padded to the 4KiB boundary, or they will not be accepted. PocketMine-MP already performs this padding, so you don't need to worry about that.
  • A check for header length has been added. Since fread() stops reading at eof, it's possible to get a bad region that's smaller than the header size, which will throw out lots of undefined offset errors. The server will now reject region files smaller than 8192 bytes.
  • Every offset in the location table is now checked to make sure that
    • it points to a valid location
    • it's not duplicated (this would cause chunks to be overwritten, bad)

If any of the detailed checks fail, the region file will be considered corrupted, will be renamed with a backup name, and a new one will be generated. You may then attempt to use MCEdit or other tools to attempt to repair the defective region.

@dktapps dktapps added Category: Core Related to internal functionality Type: Fix Bug fix, typo fix, or any other fix Priority: Critical labels Mar 2, 2017
Check size, check header size, check location table offsets point to valid locations, check for shared offsets, prevent issues with corrupted or junk data
@dktapps dktapps merged commit 2a59977 into master Apr 27, 2017
@dktapps dktapps deleted the region-safety branch April 27, 2017 08:14
dktapps added a commit that referenced this pull request Apr 27, 2017
Check size, check header size, check location table offsets point to valid locations, check for shared offsets, prevent issues with corrupted or junk data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Core Related to internal functionality Priority: Critical Type: Fix Bug fix, typo fix, or any other fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants