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

[Thomas Stuefe]

On Mon, 10 Nov 2025 20:03:35 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 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.

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.

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.

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?

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2527097852
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2527104017
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2527186265
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2527153855
PR Review Comment: https://git.openjdk.org/jdk/pull/27685#discussion_r2527159978