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

[Johan Sjölen]

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

>> 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
>
> Kept. to be used in gtests.

See comment about the `using` statements.

>> 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?
>
> return values removed.

Then we're doing a lot of special code in the implementation only for the purpose of testing. That's going to be quite surprising for the next reader. If anything, we *should* use these typedefs in the implementation.

>> 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?
>
> In `.../clang/15.0.0/include/sanitizer/asan_interface.h`, the ASAN_(UN)POISON_MEMORY_REGION  macros would be empty as  
> ```C++
> // Macros provided for convenience.
> #if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
> /// Marks a memory region as unaddressable.
> ///
> /// \note Macro provided for convenience; defined as a no-op if ASan is not
> /// enabled.
> ///
> /// \param addr Start of memory region.
> /// \param size Size of memory region.
> #define ASAN_POISON_MEMORY_REGION(addr, size) \
>   __asan_poison_memory_region((addr), (size))
> 
> /// Marks a memory region as addressable.
> ///
> /// \note Macro provided for convenience; defined as a no-op if ASan is not
> /// enabled.
> ///
> /// \param addr Start of memory region.
> /// \param size Size of memory region.
> #define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
>   __asan_unpoison_memory_region((addr), (size))
> #else
> #define ASAN_POISON_MEMORY_REGION(addr, size) \
>   ((void)(addr), (void)(size))
> #define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
>   ((void)(addr), (void)(size))
> #endif
> 
> 
> I couldn't find yet why is that. So a fast/certain solution was to define the `__SANITIZE_ADDRESS__` explicitly.
> Should be found before integrating this PR.

Surely if `INCLUDE_ASAN` is true/present, then the definitions of `ASAN_POISION_MEMORY_REGION` etc is also present? Or are you saying that the tests failed without this code?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2431470281
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2431458817
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2431478214