RFR: 8358597: [asan] Buffer overflow in ArchiveBuilder::make_shallow_copy with Symbols

[Ioi Lam]

On Fri, 26 Sep 2025 12:13:47 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> The bug: when Symbols are copied into the dynamic CDS archive, extra padding bytes may be copied, which triggers "buffer overflow" errors in asan.
>> 
>> The fix is to copy the exact number of bytes for Symbols.
>> 
>> Since `ArchiveBuilder::make_shallow_copy()` deals with different alignments and sizes, I renamed the variables and added comments/asserts to make the code more readable.
>
> Hi Ioi,
> 
> This change looks like it solves the bug. I was thinking about this, it seems that we're already doing some special-casing when looking at `MSO::ClassType`. Perhaps it would be good to have all special-cases at the top and have them 'hug' together? Below is a variant of your fix that does that. AFAIU it's "well-known" that AOTCache zeroes out all memory allocated, so we don't need to mention that in the comment.
> 
> ```c++
>   address src = src_info->source_addr();
>   int bytes_needed = src_info->size_in_bytes();
>   char* dest;
>   char* oldtop;
>   char* newtop;
> 
>   oldtop = dump_region->top();
> 
>   if (src_info->msotype() == MetaspaceObj::ClassType) {
>     // Allocate space for a pointer directly in front of the future InstanceKlass, so
>     // we can do a quick lookup from InstanceKlass* -> RunTimeClassInfo*
>     // without building another hashtable. See RunTimeClassInfo::get_for()
>     // in systemDictionaryShared.cpp.
>     Klass* klass = (Klass*)src;
>     if (klass->is_instance_klass()) {
>       SystemDictionaryShared::validate_before_archiving(InstanceKlass::cast(klass));
>       bytes_needed += sizeof(address);
>     }
>     // Allocate space for the future InstanceKlass with proper alignment
>     const size_t alignment =
> #ifdef _LP64
>       UseCompressedClassPointers ?
>         nth_bit(ArchiveBuilder::precomputed_narrow_klass_shift()) :
>         SharedSpaceObjectAlignment;
> #else
>       SharedSpaceObjectAlignment;
> #endif
>   } else if (src_info->msotype() == MetaspaceObj::SymbolType) {
>     // Symbols may be allocated by using AllocateHeap, so their sizes
>     // may be less than size_in_bytes() indicates.
>     bytes_needed = ((Symbol*)src)->byte_size();
>   } else {
>     // All other cases need no special-handling.
>   }
>   dest = dump_region->allocate(bytes_needed);
>   newtop = dump_region->top();
> 
>   memcpy(dest, src, bytes_needed);

Thanks @jdksjolen @MBaesken for the review

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

PR Comment: https://git.openjdk.org/jdk/pull/27508#issuecomment-3404002587