RFR: JDK-8275320: NMT should perform buffer overrun checks [v2]

[Thomas Stuefe]

On Thu, 18 Nov 2021 12:28:55 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>> 
>>  - Merge
>>  - Let NMT do overflow detection
>
> src/hotspot/share/services/mallocTracker.cpp line 174:
> 
>> 172:   // we check here are the bare minimum of what we know will malloc() give us
>> 173:   // (which is 64-bit even on 32-bit platforms).
>> 174:   if (!is_aligned(this, sizeof(uint64_t))) {
> 
> Where does this information come from? As far as I can see, the man-page of `malloc()` only mentions:
> 
>> "malloc returns a pointer which is suitably aligned for any built-in  type"
> 
> Why is this 64 bit on a 32-bit platform?

We know that the alignment has to be *at least* 64-bit since we know we have 64-bit inbuilt types on both 64-bit and 32-bit platforms (`uint64_t`). From experience, I know it is probably more, 16 or 32 bytes. This makes sense since there exist scalar data types larger than 64-bit.

But this code tests the *minimal* necessary alignment only and I wanted to prevent false positives. Let's say, in case we happen to run with a libc whose malloc implementation returns only 64-bit aligned pointers. That also could happen if someone put a weird malloc() implementation below us (malloc hooks or LD_PRELOAD). I think the assumption that everything malloc() returns is at least 64-bit aligned is pretty safe though.

Ideally, we would have a clear definition of malloc alignment somewhere in `globalDefinitions.hpp`. In hotspot, there are a range of places where such alignment is implicitly assumed. The NMT header size, for instance, or metaspace allocation size, hotspot arena allocation alignment etc. Basically, everywhere where one either marshalls malloc'ed blocks or implements some sort of general purpose allocator. C++ has `std::max_align_t` for that. Maybe we could use that one. But that's a topic for another RFE.

I try to improve the comment.

> src/hotspot/share/services/mallocTracker.hpp line 314:
> 
>> 312:   static const uint8_t  _footer_canary_dead_mark = 0xFB;
>> 313:   NOT_LP64(static const uint32_t _header_alt_canary_life_mark = 0xFAFA1F1F;)
>> 314:   NOT_LP64(static const uint32_t _header_alt_canary_dead_mark = 0xFBFB1F1F;)
> 
> Just out of interest, how did you choose these canary marks? Is there some evidence that they appear less frequently in real code/data than other values?

I did an extensive statistical analysis of many core dumps.

...

...

Just kidding, I chose them on a whim to be not zero :) Do you have a better suggestion? I thought about making them ASCII pattern, but those are actually more common in payload data.

> test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 71:
> 
>> 69: // this should generate two hex dumps, one with the front header, one with the overwritten
>> 70: // portion.
>> 71: static void test_overwrite_back_long() {
> 
> I think the test isn't really checking that we get two hex dumps, right?

Again, bad wording in the comment. This tests that `MallocHeader::print_block_on_error()` prints a hex dump covering both header and the corruption address, and if both are too far apart, that the dump is split up in two parts.

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

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