RFR: 8237521: Memory Access API fixes for 32-bit

[David Holmes]

Hi Nick,

On 24/01/2020 3:32 pm, Nick Gasson wrote:
> Hi David,
> 
> On 01/24/20 12:17 pm, David Holmes wrote:
>>>
>>> That said, memory segments are not the only clients of
>>> Unsafe::allocateMemory - so if we decided that overflow is an issue
>>> that should be handled in clients, fine, but at least it should be
>>> evident somewhere in the javadoc of Unsafe::allocateMemory?
>>
>> I'm glad you raised that as it prompted me to examine Unsafe in more
>> detail. :)
>>
>> The onus is put on the caller to ensure arguments are valid. Unsafe
>> class docs state, and a number of methods like allocateMemory repeat:
>>
>>       * <em>Note:</em> It is the resposibility of the caller to make
>>       * sure arguments are checked before the methods are called. While
>>       * some rudimentary checks are performed on the input, the checks
>>       * are best effort and when performance is an overriding priority,
>>       * as when methods of this class are optimized by the runtime
>>       * compiler, some or all checks (if any) may be elided. Hence, the
>>       * caller must not rely on the checks and corresponding
>>       * exceptions!
>>
>> So although allocateMemory explicitly checks for size_t related overflow:
>>
>>       * @throws RuntimeException if the size is negative or too large
>>       *         for the native size_t type
>>
>> the caller should not be relying on that.
>>
>> The existing overflow check is done by checkSize(long size) which has:
>>
>>   if (ADDRESS_SIZE == 4) {
>>      // Note: this will also check for negative sizes
>>      if (!is32BitClean(size)) {
>>          throw invalidInput();
>>      }
>>   }
>>
>> where is32BitClean is defined as:
>>
>> value >>> 32 == 0;
>>
>> and so is incomplete as this doesn't take into account the possible
>> align up of "size" that the JVM allocation routine actually
>> performs. But even if we fix this, the caller is not supposed to rely on
>> it.
>>
>> In terms of fixing it ... it isn't clear to me whether this case should
>> be detected in the VM as Nick's patch does, or whether it should be
>> caught before hand in the checkSize Java code (though we don't really
>> know what the alignment requirement may be in the Java code). A
>> difference is that if we catch it in checkSize then we will throw
>> IllegalArgumentException, whereas Nick's approach results in
>> OutOfMemoryError. I think the IllegalArgumentException is actually
>> preferable here.
>>
> 
> How about we align the size up to ADDRESS_SIZE (== HeapWordSize) in
> Unsafe.allocateMemory() before the call to allocateMemoryChecks(). Like:
> 
>    bytes = ((bytes + ADDRESS_SIZE - 1) & ~(ADDRESS_SIZE - 1));
> 
> Then it will throw an IllegalArgumentException if the result is outside
> the size_t range. And in the native code we can just assert that the
> `size' argument is already aligned:
> 
>    assert(is_aligned(sz, HeapWordSize), "sz not aligned");

That seems quite reasonable.

Thanks,
David

> 
> Thanks,
> Nick
>