RFR: JDK-8275320: NMT should perform buffer overrun checks

[Zhengyu Gu]

On Thu, 14 Oct 2021 15:49:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> This is part of a number of RFE I plan to improve and simplify C-heap overflow checking in hotspot. For the whole story please refer to https://bugs.openjdk.java.net/browse/JDK-8275301.
> 
> This proposal adds NMT buffer overflow checking. As laid out in JDK-8275301:
> 
> - it would give us C-heap overflow checking in release builds
> - the additional costs are neglectable
> - NMT needs intact headers anyway. Faced with buffer overwrites today, it would maybe crash or maybe account wrongly, but it's a bit of a lottery really. The error reports would also be confusing.
> - it is a preparation for future code removal (the memory guarding done in debug only in os::malloc() and friends, and possibly the guarding done with CheckJNICalls)
> 
> Patch notes:
> 
> 1) The malloc header is changed such that it contains a 16-bit canary directly preceding the user payload of the allocation. 
> 
> On 64-bit, we don't even need to enlarge the malloc header: we carve some bits out by decreasing the size of the bucket index bit field to 16 bits. The bucket index field is used to store the bucket slot of the malloc site table in NMT detail mode. The malloc site table width is 512 atm, so 65k gives plenty of room for growing the malloc site table should we ever want to.
> 
> On 32-bit, I had to enlarge the header from 8 bytes to 16 bytes. That is because there were not enough bits to spare for a canary. On the upside, 8 bytes were not enough anyway, strictly speaking, to guarantee proper alignment e.g. for 128bit data types on all 32-bit platforms. See e.g. the malloc alignment the glibc uses.
> 
> I also took the freedom of re-arranging the malloc header fields a bit to minimize the difference between 32-bit and 64-bit platforms, and to align each field optimally according to its size. I also switched from bitfields to real types in order to be able to do a sizeof() on them.
> 
> For more details, see the comment in mallocTracker.hpp.
> 
> 2) I added a footer canary trailing the user allocation to catch tail buffer overruns. For simplicity reasons (alignment) and to save some cycles I made it a byte only. That is enough to catch most overrun scenarios. If you think this is too small, I'm open to change it.
> 
> 3) I put a bit of work into error reporting. When NMT detects corruption, it will now print out a hex dump of the corrupted area to tty before asserting.
> 
> 4) I added a bunch of gtests to test various heap overwrite scenarios. I also had to extend the gtest macros a bit because I wanted these tests of course to run in release builds too, but we did not have a death test macro for release builds yet (there are possibilities for code simplification here too, but that's for another RFE).
> 
> (Note that these gtests, to test anything, need to run with NMT switched on. We do this as part of our NMT jtreg-controlled gtests in tier1).
> 
> Even though the patch adds more code than it removes, it prepares possible code removal (if we can agree to do that) and the net result will be less complexity, not more. Again, see JDK-8275301 for details.
> 
> --------------
> 
> Example output a buffer overrun would provide:
> 
> 
> Block at 0x00005600f86136b0: footer canary broken at 0x00005600f86136c1 (buffer overflow?)
> NMT Block at 0x00005600f86136b0, corruption at: 0x00005600f86136c1: 
> 0x00005600f86136a8:   21 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00
> 0x00005600f86136b8:   00 00 00 00 0f 00 1f fa 00 61 00 00 00 00 00 00
> 0x00005600f86136c8:   41 39 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f86136d8:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 0x00005600f86136e8:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f86136f8:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f8613708:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f8613718:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f8613728:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00005600f8613738:   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> assert failed: fatal error: Block at 0x00005600f86136b0: footer canary broken at 0x00005600f86136c1 (buffer overflow?)#
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (mallocTracker.cpp:203), pid=10805, tid=10805
> #  fatal error: Block at 0x00005600f86136b0: footer canary broken at 0x00005600f86136c1 (buffer overflow?)
> #
> 
> -------
> 
> Tests:
> - manual tests with Linux x64, x86, minimal build
> - GHAs all clean
> - SAP nightlies in progress.

Sorry, we already have GuardedMemory for detecting buffer overrun, why introduce a new one?

-------------

PR: https://git.openjdk.java.net/jdk/pull/5952