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 <#284 (comment)>, 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