RFR: 8369393: NMT: poison the malloc header and footer under ASAN build [v2]

[Afshin Zafari]

On Tue, 2 Dec 2025 07:42:39 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Afshin Zafari 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 eight additional commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'origin/master' into asan_poison_malloc_hdr_ftr_v2
>>  - removed extra newlines.
>>  - fixes.
>>  - inlining
>>  - review comments applied.
>>  - revision
>>  - jtreg test excluded when ASAN is enabled.
>>  - 8369393: NMT: poison the malloc header and footer under ASAN build
>
> src/hotspot/share/nmt/mallocHeader.cpp line 80:
> 
>> 78: }
>> 79: 
>> 80: MallocHeader* MallocHeader::kill_block(void* memblock) {
> 
> Put these functions in the inline file instead, seems unnecessary to not get inlining of these calls.

These functions are not templated. When put them in inline file, the definitions will be repeated for every cpp that include the inline file. There are 3 instances of these cpp files. Then, linker complains about duplicate definitions.

> src/hotspot/share/nmt/mallocHeader.cpp line 83:
> 
>> 81:   MallocHeader* header = (MallocHeader*)memblock - 1;
>> 82:   ASAN_UNPOISON_MEMORY_REGION(header, sizeof(MallocHeader));
>> 83:   ASAN_UNPOISON_MEMORY_REGION(header->footer_address(), sizeof(uint16_t));
> 
> We spread around the knowledge of the size of the footer being 16 bytes by adding this. We should have one place which tells us the size of the footer (maybe a `constexpr size_t footer_size;` stmt in the class header?) and use that only.

Done.

> src/hotspot/share/nmt/mallocHeader.cpp line 95:
> 
>> 93:   ASAN_POISON_MEMORY_REGION(header, sizeof(MallocHeader));
>> 94:   return header;
>> 95: }
> 
> Here you missed that we shouldn't return the header as it's invalid.

changed func to void.

> src/hotspot/share/runtime/os.cpp line 716:
> 
>> 714:     assert(mem_tag == header->mem_tag(), "weird NMT type mismatch (new:\"%s\" != old:\"%s\")\n",
>> 715:            NMTUtil::tag_to_name(mem_tag), NMTUtil::tag_to_name(header->mem_tag()));
>> 716:     bool success = false;
> 
> Here's an alternative variant where `success` is assigned before branches and the conditions are given names (instead of having comments).
> ```c++
>     // Perform integrity checks on and mark the old block as dead *before* calling the real realloc(3) since it
>     // may invalidate the old block, including its header.
>     assert(mem_tag == header->mem_tag(), "weird NMT type mismatch (new:"%s" != old:"%s")\n",
>            NMTUtil::tag_to_name(mem_tag), NMTUtil::tag_to_name(header->mem_tag()));
>     bool within_malloc_limit = !((size > old_size) && MemTracker::check_exceeds_limit(size - old_size, mem_tag));
>     bool success = within_malloc_limit;
>     if (success) {
>       void* const new_outer_ptr = permit_forbidden_function::realloc(header, new_outer_size);
>       bool realloc_succeeded = new_outer_ptr != nullptr;
>       success = realloc_succeeded;
>       if (success) {
>         // realloc(3) succeeded, variable header now points to invalid memory and we need to deaccount the old block.
>         MemTracker::deaccount(free_info);
>         // After a successful realloc(3), we account the resized block with its new size
>         // to NMT.
>         void* const new_inner_ptr = MemTracker::record_malloc(new_outer_ptr, size, mem_tag, stack);
> 
> #ifdef ASSERT
>         if (ZapCHeap && old_size < size) {
>           // We also zap the newly extended region.
>           ::memset((char*)new_inner_ptr + old_size, uninitBlockPad, size - old_size);
>         }
> #endif
> 
>         rc = new_inner_ptr;
>       }
>     }

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28503#discussion_r2580553465
PR Review Comment: https://git.openjdk.org/jdk/pull/28503#discussion_r2580544542
PR Review Comment: https://git.openjdk.org/jdk/pull/28503#discussion_r2580542971
PR Review Comment: https://git.openjdk.org/jdk/pull/28503#discussion_r2580540854