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.
