RFR: 8369393: NMT: poison the canaries of malloc header under ASAN build [v7]

[Johan Sjölen]

On Tue, 14 Oct 2025 11:01:06 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> NMT can detect malloc'd memory corruption using canary tests at header and footer of every memory region. This can only be done at free time of the memory where NNT checks the canaries and report error if they are not as expected.
>> In this PR, the canary parts also are poisoned using ASAN API to get notified whenever a read/write op is done. on the canary parts. `_size` member of the malloc header is also poisoned, since it is used for finding the footer address.
>
> Afshin Zafari has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - better style
>  - a step back
>  - alternative impl

Changes requested by jsjolen (Reviewer).

src/hotspot/share/nmt/mallocHeader.hpp line 108:

> 106:     ASAN_UNPOISON_MEMORY_REGION(addr, sizeof(T));
> 107:   }
> 108: };

Wrap the bodies of these functions with the `#ifdef ASAN`. As these defs are in the header, they'll be visible to any usages by the compiler, and so can be optimized away when ASAN is not in use. This helps simplify the rest of the code.

The name `Poisoner` is unfortunate, as it's technically `Unpoisoning` the memory region. You can also have the `_memory` be `T*` and have the class take `T*` as its argument and do the casting inside of the class body instead. This avoids unnecessary clutter in the user-code.

Perhaps rename `_memory` to `_memory_region` to mimic the wording used in the ASAN macros?

Use `reinterpret_cast` instead of C-style cast, with the intent of providing the full meaning of the cast.


`register` and `unregister` memory, perhaps they should be poison and unpoison?

src/hotspot/share/nmt/mallocHeader.hpp line 120:

> 118:   static void unregister_memory(char* addr) { }
> 119:   ~AsanPoisoner() { }
> 120: };

I think you can delete this definition

src/hotspot/share/nmt/mallocHeader.hpp line 130:

> 128:   using SizeType = void;
> 129:   NOT_LP64(using AltCanaryType = void;)
> 130: #endif

Surely this won't work, as we're using some of these types as return types in functions?

src/hotspot/share/nmt/mallocTracker.cpp line 191:

> 189:   assert(((size_t)memblock & (sizeof(size_t) * 2 - 1)) == 0, "Alignment check");
> 190: 
> 191: 

Style: Added in space, remove

src/hotspot/share/sanitizers/address.hpp line 29:

> 27: 
> 28: #ifdef ADDRESS_SANITIZER
> 29: #define __SANITIZE_ADDRESS__

Why not just check for `ADDRESS_SANITIZER` in the test, and skip this definition?

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

PR Review: https://git.openjdk.org/jdk/pull/27685#pullrequestreview-3335143623
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2428844050
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2428834787
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2428834342
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2428848537
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2428850122