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

[Afshin Zafari]

On Tue, 25 Nov 2025 14:21:36 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   pragma fix
>
> Hi @afshin-zafari , 
> 
> In general, I support the goal of this PR, but in its current form, it incurs too high a cost for the added complexity. 
> 
> The benefit this PR provides is that, with NMT enabled, we won't catch 1-2-byte overwriters at the end and up to 16-byte underwriters at the start of a block. Underwriters are very rare; overwriters are a lot more common. So, if someone overwrites by one or two bytes, today we only see it if the block is free'd. If someone overwrites three bytes, we already have protection. With your patch, we see also 1-2 byte overwriters.
> 
> But there is a straightforward workaround: just disable NMT. Then, the protection extends right to the borders of the user payload.
> 
> Considering that, the benefit of this PR is there, but it's not huge. Therefore, the complexity cost should be correspondingly low.
> 
> As I wrote before, I don't think poisoning and unpoisoning in this fine-grained fashion is the best approach. It often makes little sense. For example, in os::malloc we have at least one poison-unpoison-poison cycle, possibly more. It would be a lot simpler to do this carefully and only where it's needed:
> 
> - poison the malloc header and footer when we *hand out a malloc'ed block* to the user *at the end of*  `os::malloc` or `os::realloc`
> - unpoison the header and footer *on the entry* to `os::realloc` and `os::free` to be able to read the MST marker.
> 
> That's it. No more poisoning should be needed.
> 
> NMT block integrity checks should simply not run for ASAN. They make zero sense. They check header corruptions, but who is going to corrupt the canary if the header is now protected by ASAN? In fact, we should not even need to initialize the canaries.
> 
> Basically, that means not calling either one of `check_block_integrity` or `is_valid_malloced_pointer` or `print_block_on_error` if ASAN is enabled. Resp. stubbing them out for ASAN. But I see only one call site that does this. Ideally, one would have a new define like "NMT_BLOCK_INTEGRITY_CHECKS", and guard the NMT integrity checks with that one, and switch it off for ASAN.
> 
> That means that a large part of this PR can be simplified. Many functions can stay as they are, and there should be no need for an RAII object like `AsanPoisoningHelper`. We should only have two poison calls for the header and footer, and two unpoison calls, none of which share the same extent, so RAII can't be used.
> 
> Other than that, there are many cosmetic changes in this PR, e.g., many accessor functions added. That ...

Dear reviewers @tstuefe , @jdksjolen and @Arraying ,
This PR is closed and a [new](https://github.com/openjdk/jdk/pull/28503) one is made from a clean branch.
Thanks in advance for your comments on it.

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

PR Comment: https://git.openjdk.org/jdk/pull/27685#issuecomment-3664320129