RFR: JDK-8295865: Several issues with os::realloc

[David Holmes]

On Tue, 25 Oct 2022 16:27:37 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> There are several issues with os::realloc():
> 
> 1) If realloc(3) fails, the original block will be left untouched. That is fine, and the caller may expect this and continue to use the old block, including handing it to os::free() later. But NMT has marked the original block as dead already, and subsequent os::free() or os::realloc() will trigger a false fatal block integrity error. Therefore, if realloc(3) fails, we need to revive the NMT header and re-account the original block before returning.
> 
> 2) If handed in very large sizes, the size may overflow if the NMT header is added. The result would be that the VM reallocates to a much smaller buffer which would cause subsequent memory corruption if the caller were to use the buffer (same as JDK-8286519, but for realloc).
> 
> 3) If os::realloc() enlarges a buffer, the newly added memory should be zapped with uinitBlockPad as we do for os::malloc(). Of course we only can do this if NMT is enabled, otherwise we won't know the original block size.
> 
> ------------------------------
> 
> The patch fixes all three issues and adds hopefully thorough enough regression gtests for them. Remember that these gtests will run, as part of the jtreg gtest runners, in all NMT modes.
> 
> The largest diff hunk is inside os::realloc(). Note that I separated the two cases NMT=summary/detail and NMT=off, since it makes the coding easier to understand and we also run less code in the standard case of NMT==off.

This all appears to be quite reasonable.

I have a couple of queries.

Thanks.

src/hotspot/share/runtime/os.cpp line 732:

> 730:       // NMT header then, since we marked it dead already. Otherwise subsequent os::realloc()
> 731:       // or os::free() calls would trigger block integrity asserts.
> 732:       void* p = MemTracker::record_malloc(old_outer_ptr, old_size, memflags, stack);

I'm not familiar with NMT details - are `memflags` and `stack` guaranteed to be the same as the original allocation?

test/hotspot/gtest/testutils.hpp line 57:

> 55: #define ASSERT_NULL(p)      ASSERT_EQ(p2i(p), 0)
> 56: #define EXPECT_NOT_NULL(p)  EXPECT_NE(p2i(p), 0)
> 57: #define EXPECT_NULL(p)      EXPECT_EQ(p2i(p), 0)

These aren't "convenience asserts" - should they be somewhere else? What does EXPECT_NULL actually do?

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

PR: https://git.openjdk.org/jdk/pull/10857