RFR: JDK-8298908: Instrument Metaspace for ASan [v6]

[Justin King]

On Mon, 9 Jan 2023 07:39:03 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Justin King has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use -fno-common as suggested by ASan docs
>>   
>>   Signed-off-by: Justin King <jcking at google.com>
>
> src/hotspot/share/memory/metaspace/chunkManager.cpp line 137:
> 
>> 135: // - Or, if the necessary space cannot be committed because we hit a commit limit.
>> 136: //   This may be either the GC threshold or MaxMetaspaceSize.
>> 137: Metachunk* ChunkManager::get_chunk_locked(chunklevel_t preferred_level, chunklevel_t max_level, size_t min_committed_words) {
> 
> Please add `assert_lock_strong(Metaspace_lock);`

Done.

> src/hotspot/share/memory/metaspace/chunkManager.cpp line 247:
> 
>> 245: //    this function returns !!
>> 246: void ChunkManager::return_chunk(Metachunk* c) {
>> 247:   ASAN_POISON_MEMORY_REGION(c->base(), c->word_size() * BytesPerWord);
> 
> Please add comment:
> 
> // It is valid to poison the chunk payload area at this point since its physically separated from the chunk meta info.

Done.

> src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 239:
> 
>> 237:   assert_is_aligned(_word_size, chunklevel::MAX_CHUNK_WORD_SIZE);
>> 238: 
>> 239:   // Poison the memory region. It will be unpoisoned later by MetaspaceArena.
> 
> Can you be a bit more specific? Proposal:
> "It will be unpoisened on a per-chunk base for chunks that are handed to arenas."

Done.

> src/hotspot/share/runtime/os.cpp line 951:
> 
>> 949: NO_SANITIZE_ADDRESS void os::print_hex_dump(outputStream* st, address start, address end,
>> 950:                                             int unitsize, int bytes_per_line,
>> 951:                                             address logical_start) {
> 
> Out of scope for this bug; can this be moved to a different RFE? 
> 
> While looking at this, I found that os::print_hex_dump has a little error. It is not supposed to access memory directly but only via SafeFetch (in order to print '?' for unreadable locations). SafeFetch would be the natural point for asan exclusion. I filed a bug for this: https://bugs.openjdk.org/browse/JDK-8299790

Sure, reverted the changes to this file. I think SafeFetch is fine already as its assembly and instrumentation won't happen, but we can double check later once its fixed. I think there are some non-assembly versions of SafeFetch and that would need to be fixed.

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

PR: https://git.openjdk.org/jdk/pull/11702