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
