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");
Thanks,
Nick
