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

[Afshin Zafari]

On Fri, 14 Nov 2025 11:04:50 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   improvements.
>
> src/hotspot/share/sanitizers/address.hpp line 80:
> 
>> 78: 
>> 79: template<typename T>
>> 80: class AsanPoisoningHelper {
> 
> It would be preferable not to have it as a template, since one may want to use this with memory ranges whose sizes are not known at compile time. Just make the size a const member.
> 
> In addition to that, I am unsure we want to make it this easy to protect infinitesimal small memory ranges (e.g. uint16_t) before we have an idea of the overhead costs of every poison call. Especially if this is meant to be a general purpose class, not only for NMT malloc and free.

Template is removed. ctor gets ptr and size now.
Poisoning memory blocks is  as easy/difficult as calling `ASAN_POISON_MEMORY_REGION(ptr, size)` macro.

> src/hotspot/share/sanitizers/address.hpp line 81:
> 
>> 79: template<typename T>
>> 80: class AsanPoisoningHelper {
>> 81:   const T* _memory_region;
> 
> The pointer should be const.

I think I did not understand this comment. The pointer is already const. Which pointer do you refer to?

> src/hotspot/share/sanitizers/address.hpp line 85:
> 
>> 83:   AsanPoisoningHelper() = delete;
>> 84:   AsanPoisoningHelper(const T* addr) : _memory_region(addr) {
>> 85:     #if INCLUDE_ASAN
> 
> We use ADDRESS_SANITIZER in other places in this file, so I would use that.
> 
> In addition, I would wrap the whole class into ADDRESS_SANITIZER and not do this for the individual functions. And provide a simple stub for the non-ASAN case.

Done.

> test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 184:
> 
>> 182: 
>> 183: #else // ASAN is enabled
>> 184: 
> 
> Do we really need to duplicate this macro, just to give the test an own name? Certainly this can be simplified?

Fixed.

> test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 202:
> 
>> 200:   *canary_ptr = 1;
>> 201: }
>> 202: 
> 
> If we simplify to poisoning the whole header (or just the canary itself) this whole section can be shrunk down to two test functions.
> 
> Otherwise, there is a large potential for simplification here. A lot of that code is duplicated.
> 
> Also note that there is a time cost involved with very fine-grained tests like these. Death tests take a long time, since they involve forking new processes, starting the JVM, letting it crash, and writing the hs_err.log file. Seeing that we basically just test different offsets in the header, the usefulness of this many tests is questionable. 
> 
> I think I would reduce this to just one test, or maybe two if you want to be thorough and test both start and end of header. Arguably the only test really needed is the test for the canary, since it borders the user section directly and is the first to be hit on a buffer overflow.
> 
> As I wrote before: the section *preceding* the header is protected by ASAN anyway.

The extra tests removed. The number of tests are as {header, footer} x {read, write} x {realloc, malloc} == 8 cases now.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2541239081
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2541260205
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2541254073
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2541239883
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2541247376