Crash while parsing corrupt NBT data

[EtlamGit]

We crash when corrupt NBT data is present. Example Region file to trigger that crash: NBT_crash.zip Affected Chunk: -77 / -105

Expected behavior would be to detect that and ignore that Chunk.

NBTExplorer also crashes -> fail Minecraft generates some exceptions and after a while regenerates that Chunk -> expected behavior

I spend some time debugging, but did not find a way to detect that.

Debugger will tell you, that QtConcurrent has failed with an unhandled exception, but in real it crashes in NBT::NBT(const uchar *chunk) somewhere after line 66.

I placed some breakpoints to ease debugging:

• ChunkLoader::run() line 21: if ((cx == -77) && (cz == -105)) {}
• NBT::NBT(const uchar *chunk) line 77: if (strm.total_out == 35170) {}

[EtlamGit]

It seems that for this Chunk the NBT data is too short, not written completely. This results in root = new Tag_Compound(&s); to read beyond the length of the data stream.

It works without a crash when I change this in tagdatastream.cpp:

quint8 TagDataStream::r8() {
  if (pos >= len) return 0;
  return data[pos++];
}

Any opinion about performance impact when we add such safety checks for all TagDataStream::readXYZ methods?

[mrkite]

Since pos and len are both just integers, that safety check should be pretty negligible in terms of performance impact.

On Sun, Feb 6, 2022 at 10:35 AM EtlamGit @.***> wrote:

It seems that for this Chunk the NBT data is too short, not written completely. This results in root = new Tag_Compound(&s); to read beyond the length of the data stream.

It works without a crash when I change this in tagdatastream.cpp:

quint8 TagDataStream::r8() { if (pos >= len) return 0; return data[pos++]; }

Any opinion about performance impact when we add such safety checks for all TagDataStream::readXYZ methods?

— Reply to this email directly, view it on GitHub https://github.com/mrkite/minutor/issues/284#issuecomment-1030878514, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGASZ43QDZ5QENL2ZHVDA3UZ2WNTANCNFSM5NUKRZXA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

[EtlamGit]

I did some profiling: no significant difference is measurable. (Test is to batch process: load a world and save it as PNG, average 51.5s, deviation 1/3s, neither is faster than the other)

-> I will prepare a fix