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

[David Holmes]

Hi Maurizio,

On 24/01/2020 9:27 am, Maurizio Cimadamore wrote:
> 
> On 23/01/2020 22:59, David Holmes wrote:
>> On 24/01/2020 12:41 am, Andrew Haley wrote:
>>> On 1/23/20 10:22 AM, David Holmes wrote:
>>>> That aside IIRC the overflow check is not ideal here because we already
>>>> enter undefined behaviour territory inside align_up if we actually
>>>> overflow.
>>>
>>> How is that possible? size_t is an unsigned type, and unsigned types
>>> never overflow.
>>
>> Yes you are right, I was not recalling the rules correctly. So the 
>> addition to the size_t is guaranteed to wrap and so the < test is valid.
>>
>> Sorry for the noise on that.
>>
>> It may still be cleaner if the Java side enforces a maximum of 
>> Integer.MAX_VALUE for 32-bit.
> 
> I have no objections in adding an extra check where the native memory 
> segment is created - perhaps we should just do that.
> 
> 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.

David
-----

> Maurizio
> 
>>
>> Thanks,
>> David
>>
>>> On a 32-bit box, 0 <= size < 2**32. So -- in theory at
>>> least -- you could allocate more than 2G.
>>>